Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1467493002:
We mustn't finalize sweeping when V8 started collecting garbage. (Closed)
|
Created:
5 years, 1 month ago by jochen (gone - plz use gerrit) Modified:
5 years, 1 month ago CC:
Mads Ager (chromium), blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, kouhei+heap_chromium.org, oilpan-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWe mustn't finalize sweeping when V8 started collecting garbage.
The finalizers might call into V8 which is not safe to do. There's
actually a script forbidden scope on the stack in the prologue, but it's
created right after the call to finialize sweeping, so it didn't catch
this before.
BUG=none
R=sigbjornf@opera.com,haraken@chromium.org
Committed: https://crrev.com/93276fb7983b7794cc533581462f7c39e623dea5
Cr-Commit-Position: refs/heads/master@{#360834}
Patch Set 1 #
Total comments: 2
Patch Set 2 : updates #
Total comments: 2
Patch Set 3 : updates #Messages
Total messages: 18 (1 generated)
https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/ThreadState.cpp:635: // The fact that the PageNavigation GC is scheduled means that there is This means we cannot do page navigation GCs either.
LGTM But we cannot simply remove the completeSweep because the completeSweep is important to collect floating garbage. Would it be possible to implement a callback that tells the embedder that V8 is going to start marking? Then we can call completeSweep() in the callback. You can land this CL at the moment since the floating garbage issue won't happen until we move Frames to Oilpan's heap (i.e., until we ship Oilpan by default).
On 2015/11/20 14:39:37, sof wrote: > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/heap/ThreadState.cpp:635: // The fact that > the PageNavigation GC is scheduled means that there is > This means we cannot do page navigation GCs either. Yeah... > The finalizers might call into V8 which is not safe to do. ^^^ What finalizes are calling into V8? With or without Oilpan, it sounds problematic...
On 2015/11/20 at 14:41:53, haraken wrote: > LGTM > > But we cannot simply remove the completeSweep because the completeSweep is important to collect floating garbage. Would it be possible to implement a callback that tells the embedder that V8 is going to start marking? Then we can call completeSweep() in the callback. as discussed, completeSweep() shouldn't be called directly from such a callback. either post a task that triggers completeSweep(), or set a flag such that completeSweep is invoked next time the message loop is reached. > > You can land this CL at the moment since the floating garbage issue won't happen until we move Frames to Oilpan's heap (i.e., until we ship Oilpan by default).
On 2015/11/20 at 14:43:03, haraken wrote: > On 2015/11/20 14:39:37, sof wrote: > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:635: // The fact that > > the PageNavigation GC is scheduled means that there is > > This means we cannot do page navigation GCs either. > > Yeah... > > > The finalizers might call into V8 which is not safe to do. > > ^^^ What finalizes are calling into V8? With or without Oilpan, it sounds problematic... ~V8AbstractEventListener does when it removes the hidden values from its wrapper
On 2015/11/20 14:43:03, haraken wrote: > On 2015/11/20 14:39:37, sof wrote: > > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:635: // The fact that > > the PageNavigation GC is scheduled means that there is > > This means we cannot do page navigation GCs either. > > Yeah... > > > The finalizers might call into V8 which is not safe to do. > > ^^^ What finalizes are calling into V8? With or without Oilpan, it sounds > problematic... .... STDERR: 14: blink::V8HiddenValue::deleteHiddenValue(blink::ScriptState*, v8::Local<v8::Object>, v8::Local<v8::String>) STDERR: 15: blink::V8EventListenerList::clearWrapper(v8::Local<v8::Object>, bool, blink::ScriptState*) STDERR: 16: blink::V8AbstractEventListener::~V8AbstractEventListener() STDERR: 17: blink::V8EventListener::~V8EventListener() STDERR: 18: blink::V8EventListener::~V8EventListener() STDERR: 19: blink::GarbageCollectedFinalized<blink::EventListener>::finalizeGarbageCollectedObject() ... (from https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpa... )
(new patchset uploaded) https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/ThreadState.cpp:635: // The fact that the PageNavigation GC is scheduled means that there is On 2015/11/20 at 14:39:37, sof wrote: > This means we cannot do page navigation GCs either. hum, right :-/
On 2015/11/20 14:46:05, jochen wrote: > On 2015/11/20 at 14:43:03, haraken wrote: > > On 2015/11/20 14:39:37, sof wrote: > > > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:635: // The fact > that > > > the PageNavigation GC is scheduled means that there is > > > This means we cannot do page navigation GCs either. > > > > Yeah... > > > > > The finalizers might call into V8 which is not safe to do. > > > > ^^^ What finalizes are calling into V8? With or without Oilpan, it sounds > problematic... > > ~V8AbstractEventListener does when it removes the hidden values from its wrapper That is what I asked here... https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour... Running an arbitrary script in a destructor seems already unsafe in a non-oilpan world :-/
On 2015/11/20 at 14:51:56, haraken wrote: > On 2015/11/20 14:46:05, jochen wrote: > > On 2015/11/20 at 14:43:03, haraken wrote: > > > On 2015/11/20 14:39:37, sof wrote: > > > > > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > > > > > > https://codereview.chromium.org/1467493002/diff/1/third_party/WebKit/Source/p... > > > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:635: // The fact > > that > > > > the PageNavigation GC is scheduled means that there is > > > > This means we cannot do page navigation GCs either. > > > > > > Yeah... > > > > > > > The finalizers might call into V8 which is not safe to do. > > > > > > ^^^ What finalizes are calling into V8? With or without Oilpan, it sounds > > problematic... > > > > ~V8AbstractEventListener does when it removes the hidden values from its wrapper > > That is what I asked here... > > https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour... > > Running an arbitrary script in a destructor seems already unsafe in a non-oilpan world :-/ it's not arbitrary, it's just that changing the layout of an object may allocate
LGTM https://codereview.chromium.org/1467493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (left): https://codereview.chromium.org/1467493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:279: Would you move this to after ScriptForbiddenScope::enter() and comment out the code with: // TODO(haraken): It is not safe to run finalizers in a prologue callback because V8AbstractEventListener's destructor can call into V8. We should post a task to schedule willStartV8GC() and avoid running it inside the prologue callback. https://codereview.chromium.org/1467493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (left): https://codereview.chromium.org/1467493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/ThreadState.cpp:633: void ThreadState::willStartV8GC() And don't remove this code :) I'll prepare a CL to call this in a posted task.
ptal
On 2015/11/20 15:18:10, jochen wrote: > ptal thanks, LGTM
The CQ bit was checked by jochen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1467493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1467493002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/93276fb7983b7794cc533581462f7c39e623dea5 Cr-Commit-Position: refs/heads/master@{#360834}
Message was sent while issue was closed.
Oilpan bots liked this one very much, thanks! :) |
