Commit 45d6949f authored by Omer Katz's avatar Omer Katz Committed by Commit Bot

Fix Null dereference in Node.cc

The Node class uses an ephemeron map to record EvenTargetData.
Each node holds a flag stating whether or not it has an entry in
the map. The map itself is only allocated when it is first
accessed and that setting the flag is always accompanied by
adding a corresponding entry to the map.
During tracing, if the flag is set, the map emtry is also traced.

This crash was due to the order of setting the flag and allocating
the map.
Existing implementation first set the flag, then allocated the map
and added an entry to it. However, since the map is GCed, allocating
it can trigger a GC (before performing the actual allocation). If
that happens, while tracing the node we will see that the flag set
try to access the map which was not yet allocated (resuling in the
Null dereference).

This CL fixes the issue by only setting the flag after adding an entry
to the map.

The following two WIP changes are also relevant to this issue:
1) Do not trace the entry when tracing the node. The entries are held in
a persistent map which is traced as a root so tracing the entries when
tracing the node is not required (this change is still WIP as it affects
DevTools heap snapshot).
2) Prohibiting Allocation during atomic phase. This crash manifested
because the map was allocated during the atomic phase. Prohibiting
allocations wouldn't directly solve the issue but, if similar issues
arise in the future, it would make it much easier to debug and resolve
them.

Bug: 993415
Change-Id: I878ee8639ea3ddbc1834ece9a9cae96a27349fea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849672Reviewed-by: default avatarMichael Lippautz <mlippautz@chromium.org>
Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704155}
parent 114c5a95
...@@ -2567,9 +2567,9 @@ EventTargetData& Node::EnsureEventTargetData() { ...@@ -2567,9 +2567,9 @@ EventTargetData& Node::EnsureEventTargetData() {
if (HasEventTargetData()) if (HasEventTargetData())
return *GetEventTargetDataMap().at(this); return *GetEventTargetDataMap().at(this);
DCHECK(!GetEventTargetDataMap().Contains(this)); DCHECK(!GetEventTargetDataMap().Contains(this));
SetHasEventTargetData(true);
EventTargetData* data = MakeGarbageCollected<EventTargetData>(); EventTargetData* data = MakeGarbageCollected<EventTargetData>();
GetEventTargetDataMap().Set(this, data); GetEventTargetDataMap().Set(this, data);
SetHasEventTargetData(true);
return *data; return *data;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment