Wrapper tracing is not communicating all wrappers to Blink |
||
Issue descriptionUpon patching in [1] quite some tests [2] on Mac fail with the stacktrace: STDERR: # STDERR: # Fatal error in ../../v8/src/heap/incremental-marking.cc, line 1005 STDERR: # Check failed: 0 == heap_->local_embedder_heap_tracer()->NumberOfCachedWrappersToTrace() (0 vs. 1). STDERR: # STDERR: 0 Content Shell Framework 0x000000010831073c base::debug::StackTrace::StackTrace(unsigned long) + 28 STDERR: 1 Content Shell Framework 0x000000010ab55c6b gin::(anonymous namespace)::PrintStackTrace() + 27 STDERR: 2 Content Shell Framework 0x000000010a76576c V8_Fatal + 220 STDERR: 3 Content Shell Framework 0x0000000107ab45f2 v8::internal::IncrementalMarking::AdvanceIncrementalMarking(double, v8::internal::IncrementalMarking::CompletionAction, v8::internal::IncrementalMarking::ForceCompletionAction, v8::internal::StepOrigin) + 1762 STDERR: 4 Content Shell Framework 0x0000000107aafe05 v8::internal::IncrementalMarkingJob::Task::RunInternal() + 165 STDERR: 5 Content Shell Framework 0x00000001083110a6 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) + 310 This DCHECK ensures that V8 always eagerly announces wrappers to Blink. Failing to do so will prohibit Oilpan from cleaning up the wrapper tracing marking deque which is necessary on a conservative Oilpan GC. The end result is that we announce the wrapper to Blink to late, effectively enqueue a dead object into the marking deque. This could result in the dispatchTraceWrappers() crashes we were observing recently. [1]: https://codereview.chromium.org/2841443005/ [2]: https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel_ng/442296/layout-test-results/results.html
,
May 2 2017
What kind of object is this (that gets deserialized with internal fields)? Maybe it's a wasm object?
,
May 3 2017
Jochen and I just went over the code together. (Thanks!) There's a a small fix on the V8-side missing to support this, see (b). In general, the deserialize() callback looks a bit brittle. We should not need to call RegisterV8Reference() from in there. (a) If the Blink-side object is already marked then SetWrapper will emit a write barrier. (b) If the V8-side object was allocated as marked (black allocation) then V8 needs to handle that. This is what is currently broken but easily fixable. For such CLs, please also CC Jochen and/or me, especially when calling into raw GC hooks.
,
May 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/2ec36b675e16693891986c10665b0e126defa10e commit 2ec36b675e16693891986c10665b0e126defa10e Author: mlippautz <mlippautz@chromium.org> Date: Thu May 04 13:10:33 2017 [heap] Report newly found wrappers after deserialization These wrappers wouldn't be found by the marker otherwise and are only reported upon the next marking step or GC which potentially is already too late; the embedder could've reclaimed those objects already. BUG= chromium:717480 Review-Url: https://codereview.chromium.org/2860753003 Cr-Commit-Position: refs/heads/master@{#45094} [modify] https://crrev.com/2ec36b675e16693891986c10665b0e126defa10e/src/heap/heap.cc
,
May 4 2017
Re #1 I'm sorry the "Design doc" link actually shows the working plan, and should be updated to describe how it works. Re #1,#2 Changes in LocalWindpwProxy.cpp are the critical parts of the project. I think the object is HTMLObject wrapper, and the code is the latter half of deserialize().
,
May 8 2017
I sync'ed repository and confirmed #4 fixed the issue. Thank you so much! I'd like to pay attention not to make situation in #3.
,
May 8 2017
Thanks for checking. Please add me to the final CL. |
||
►
Sign in to add a comment |
||
Comment 1 by mlippautz@chromium.org
, May 2 2017