Bug 108579 – [V8] Remove V8GCController::m_edenNodes and make minor DOM GC more precise

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

Currently V8GCController::m_edenNodes stores a list of nodes whose wrappers have been created since the latest GC. The reason why we needed m_edenNodes is that there was no way to know a list of wrappers in the new space of V8. By using m_edenNodes, we had been approximating 'wrappers in the new space' by 'wrappers that have been created since the latest GC'.

Now V8 provides VisitHandlesForPartialDependence(), with which WebKit can know a list of wrappers in the new space (yeah, the API name could be VisitHandlesInNewSpace() but the V8 team didn't want to expose the concept of generational GC to APIs:-). By using the API, we can remove V8GCController::m_edenNodes. The benefit is that (1) we no longer need to keep m_edenNodes and that (2) it enables more precise minor DOM GC (Remember that m_edenNodes was just an approximation).

[Kentaro Hara]

Created attachment 185940 [details]
performance benchmark

[Kentaro Hara]

Created attachment 185942 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 185942 [details]
Patch

Attachment 185942 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16296336

[WebKit Review Bot]

Comment on attachment 185942 [details]
Patch

Attachment 185942 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16298362

[Kentaro Hara]

Created attachment 185947 [details]
Patch

[Adam Barth]

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?

[Kentaro Hara]

Created attachment 185952 [details]
patch for landing

[Kentaro Hara]

(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!

[WebKit Review Bot]

Comment on attachment 185952 [details]
patch for landing

Clearing flags on attachment: 185952

Committed r141548: <http://trac.webkit.org/changeset/141548>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r141548 for reason:

userscript-plugin-document.html is flaky

Committed r141577: <http://trac.webkit.org/changeset/141577>

[Kentaro Hara]

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 ]

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 186356 [details]
Patch

[Kentaro Hara]

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).

[WebKit Review Bot]

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

[Adam Barth]

Looks like you've got some test failures to work through.  What you've written in Comment #15 makes sense.

[Kentaro Hara]

Created attachment 186506 [details]
Patch

[Kentaro Hara]

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().

[Kentaro Hara]

abarth: review?

[Adam Barth]

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.

[Adam Barth]

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.

[Kentaro Hara]

(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.

[Kentaro Hara]

Committed r141983: <http://trac.webkit.org/changeset/141983>

[Gavin Peters]

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. :-(

[WebKit Review Bot]

Re-opened since this is blocked by bug 109055

[Kentaro Hara]

Created attachment 186997 [details]
patch for landing

[Kentaro Hara]

Comment on attachment 186997 [details]
patch for landing

Corrected a place where a handle scope is created.

[Kentaro Hara]

Committed r142077: <http://trac.webkit.org/changeset/142077>

[Kentaro Hara]

Reverted r142077 for reason:

fast/filesystem/workers/file-writer-empty-blob.html is broken

Committed r142103: <http://trac.webkit.org/changeset/142103>

[Kentaro Hara]

Created attachment 187286 [details]
patch for landing

[Kentaro Hara]

Committed r142419: <http://trac.webkit.org/changeset/142419>