Summary: | [V8] Remove V8GCController::m_edenNodes and make minor DOM GC more precise | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||||||||||||
Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, gavinp, japhet, peter+ews, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 109055 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Kentaro Hara
2013-01-31 21:50:02 PST
Created attachment 185940 [details]
performance benchmark
Created attachment 185942 [details]
Patch
Comment on attachment 185942 [details] Patch Attachment 185942 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16296336 Comment on attachment 185942 [details] Patch Attachment 185942 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16298362 Created attachment 185947 [details]
Patch
Comment on attachment 185947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185947&action=review > Source/WebCore/bindings/v8/DOMDataStore.h:172 > { > ASSERT(object->wrapper().IsEmpty()); > object->setWrapper(wrapper); > - V8GCController::didCreateWrapperForNode(object); > wrapper.MakeWeak(static_cast<ScriptWrappable*>(object), weakCallback); > } We can now delete this whole function and use the ScriptWrappable version of setWrapperInObject instead. > Source/WebCore/bindings/v8/V8GCController.cpp:199 > + // The fact that we encounter a node that is not in the Eden space Should this comment still refer to Eden now that we've removed that concept? Created attachment 185952 [details]
patch for landing
(In reply to comment #6) > > We can now delete this whole function and use the ScriptWrappable version of setWrapperInObject instead. > > > Source/WebCore/bindings/v8/V8GCController.cpp:199 > > + // The fact that we encounter a node that is not in the Eden space > > Should this comment still refer to Eden now that we've removed that concept? Fixed. Thanks! Comment on attachment 185952 [details] patch for landing Clearing flags on attachment: 185952 Committed r141548: <http://trac.webkit.org/changeset/141548> All reviewed patches have been landed. Closing bug. Reverted r141548 for reason: userscript-plugin-document.html is flaky Committed r141577: <http://trac.webkit.org/changeset/141577> Comment on attachment 185952 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=185952&action=review > Source/WebCore/bindings/v8/V8GCController.cpp:262 > + ASSERT(!m_nodesInNewSpace[i]->wrapper().IsEmpty()); The following tests hit this ASSERT(), which looks strange. This ASSERT() should not be hit. userscripts/mixed-case-stylesheet.html [ Crash ] userscripts/script-run-at-start.html [ Crash ] userscripts/user-script-all-frames.html [ Crash ] userscripts/user-style-all-frames.html [ Crash ] Comment on attachment 185952 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=185952&action=review > Source/WebCore/bindings/v8/V8GCController.cpp:254 > + Node* node = V8Node::toNative(wrapper); More specifically, if we insert ASSERT(!node->wrapper().IsEmpty()), the userscripts/* tests hit the ASSERT(). Let me take a detailed look on Monday. Created attachment 186356 [details]
Patch
Comment on attachment 186356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186356&action=review > Source/WebCore/bindings/v8/V8GCController.cpp:364 > + // A minor GC can handle the main world only. > + DOMWrapperWorld* world = worldForEnteredContextWithoutContextCheck(); > + if (world && world->isMainWorld()) { abarth: Since the latest review, I just added this check. Review again please? I'm pretty sure that we need the isMainWorld() check here because a minor GC can handle the main world only. On the other hand, the reason why we cannot simply use worldForEnteredContext() to get the current world is complicated: (1) A minor GC can be triggered while the current context is being initialized. Specifically, V8DOMWindowShell::installDOMWindow() calls V8ObjectConstructor::newInstance(), which can trigger a minor GC. At this point, native information is not yet set on context->Global()->GetPrototype(). (2) The minor GC calls back V8GCController::minorGCPrologue(). Here imagine that minorGCPrologue() calls worldForEnteredContext(). (3) worldForEnteredContext() calls assertContextHasCorrectPrototype(context), which calls isWrapperOfType(toInnerGlobalObject(context), ...), which tries to retrieve native information from context->Global()->GetPrototype(). Because the native information has not yet set, the code crashes. To avoid the situation, I added another version of worldForEnteredContext() that does not call assertContextHasCorrectPrototype(context). Comment on attachment 186356 [details] Patch Attachment 186356 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16371270 New failing tests: accessibility/anchor-linked-anonymous-block-crash.html http/tests/appcache/main-resource-hash.html animations/3d/matrix-transform-type-animation.html accessibility/aria-readonly.html animations/3d/state-at-end-event-transform.html animations/animation-add-events-in-handler.html accessibility/aria-labelledby-on-input.html canvas/philip/tests/2d.clearRect.globalalpha.html accessibility/aria-disabled.html animations/animation-direction-alternate-reverse.html accessibility/aria-labelledby-overrides-label.html http/tests/appcache/access-via-redirect.php accessibility/aria-option-role.html accessibility/aria-controls-with-tabs.html http/tests/appcache/destroyed-frame.html http/tests/appcache/deferred-events-delete-while-raising-timer.html http/tests/appcache/insert-html-element-with-manifest.html accessibility/aria-link-supports-press.html animations/3d/transform-perspective.html accessibility/aria-hidden-updates-alldescendants.html accessibility/aria-scrollbar-role.html http/tests/appcache/local-content.html http/tests/appcache/manifest-parsing.html canvas/philip/tests/2d.canvas.reference.html animations/animation-controller-drt-api.html canvas/philip/tests/2d.clearRect.basic.html accessibility/aria-hidden.html http/tests/appcache/foreign-fallback.html accessibility/aria-help.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html Looks like you've got some test failures to work through. What you've written in Comment #15 makes sense. Created attachment 186506 [details]
Patch
Comment on attachment 186506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186506&action=review > Source/WebCore/bindings/v8/V8Binding.h:467 > inline DOMWrapperWorld* worldForEnteredContextIfIsolated() > { > - if (!v8::Context::InContext()) > + v8::Handle<v8::Context> context = v8::Context::GetEntered(); > + if (context.IsEmpty()) > return 0; > - return DOMWrapperWorld::isolated(v8::Context::GetEntered()); > + return DOMWrapperWorld::isolated(context); > + } > + > + inline DOMWrapperWorld* worldForEnteredContextWithoutContextCheck() > + { > + v8::Handle<v8::Context> context = v8::Context::GetEntered(); > + if (context.IsEmpty()) > + return 0; > + return DOMWrapperWorld::getWorldWithoutContextCheck(context); > } abarth: I just changed these lines, which I think fixed the test failures. Given that we are using an entered context, we have to do an empty check for the entered context instead of InContext(). abarth: review? Comment on attachment 186506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186506&action=review > Source/WebCore/bindings/v8/DOMWrapperWorld.h:74 > + static DOMWrapperWorld* getWorldWithoutContextCheck(v8::Handle<v8::Context> context) Can you add a comment about when you'd wan to call getWorldWithoutContextCheck instead of getWorld()/isolated() ? > Source/WebCore/bindings/v8/V8Binding.h:455 > + v8::Handle<v8::Context> context = v8::Context::GetEntered(); Is this safe to call if !InContext()? I guess so if we pass all the tests. >> Source/WebCore/bindings/v8/V8Binding.h:467 >> } > > abarth: I just changed these lines, which I think fixed the test failures. Given that we are using an entered context, we have to do an empty check for the entered context instead of InContext(). In what situations does InContext return true but GetEntered returns an empty context? That seems very odd. I see your explanation in Comment #15. It would be good to add a comment about that to the code because that's a very subtle issue. (In reply to comment #22) > I see your explanation in Comment #15. It would be good to add a comment about that to the code because that's a very subtle issue. Will add a comment. The weird situation happens only during an initialization step. Committed r141983: <http://trac.webkit.org/changeset/141983> This seems to cause lots of crashes in trunk. See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fcanvas%2Fwebgl%2Fgl-vertexattribpointer.html%2Cfast%2Fcanvas%2Fwebgl%2Fcontext-release-upon-reload.html%2Cfast%2Fcanvas%2Fwebgl%2Fdata-view-test.html%2Cfast%2Fcanvas%2Fwebgl%2Fframebuffer-bindings-unaffected-on-resize.html%2Cfast%2Fcanvas%2Fwebgl%2Ffunctions-returning-strings.html%2Cfast%2Fcanvas%2Fwebgl%2Fincorrect-context-object-behaviour.html%2Cfast%2Fcanvas%2Fwebgl%2Fframebuffer-object-attachment.html I'm rolling it out. :-( Re-opened since this is blocked by bug 109055 Created attachment 186997 [details]
patch for landing
Comment on attachment 186997 [details]
patch for landing
Corrected a place where a handle scope is created.
Committed r142077: <http://trac.webkit.org/changeset/142077> Reverted r142077 for reason: fast/filesystem/workers/file-writer-empty-blob.html is broken Committed r142103: <http://trac.webkit.org/changeset/142103> Created attachment 187286 [details]
patch for landing
Committed r142419: <http://trac.webkit.org/changeset/142419> |