Bug 67828 – Rework script context creation/release notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Aaron Boodman]

Rework script context creation/release notifications

[Aaron Boodman]

Created attachment 106832 [details]
Patch

[Aaron Boodman]

We have had a lot of problems with the correctness of these notifications, because we are trying to figure out information they don't send (or don't always send).

What we really want to know is:
- right after each context was created, the context and associated world id
- right before each context is released, the context and associated world id

[Aaron Boodman]

I will the older versions from the webkit api in a follow up change.

[Adam Barth]

Comment on attachment 106832 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106832&action=review

This looks fine.  My only concerns are the lack of testing and whether the new m_frame pointer will be left dangling if the Frame is destroyed before the context.  (We'll need fishd to do the final review because this patch changes the WebKit API, but we probably should iterate once first.)

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Can we add a WebKit API test that shows that we've got the right lifecycle for these notifications?

> Source/WebCore/bindings/v8/V8IsolatedContext.cpp:52
> +    : m_world(IsolatedWorld::create(worldId)), m_frame(proxy->frame())

Do we need to clear m_frame when the frame gets destroyed?  Can the context outlive the frame?

> Source/WebKit/chromium/public/WebFrameClient.h:313
> +    // Notifies that this frame's script context has been destroyed.
> +    virtual void didDestroyScriptContext(WebFrame*) { }

Should we add a comment about removing this function once the roll is complete?

[Aaron Boodman]

Created attachment 107571 [details]
Patch

[Aaron Boodman]

(In reply to comment #4)
> Can we add a WebKit API test that shows that we've got the right lifecycle for these notifications?

Yes, done.

> > Source/WebCore/bindings/v8/V8IsolatedContext.cpp:52
> > +    : m_world(IsolatedWorld::create(worldId)), m_frame(proxy->frame())
> 
> Do we need to clear m_frame when the frame gets destroyed?  Can the context outlive the frame?

I've nulled it out. This class does outlive the frame, but nobody should be calling into it once the frame is destroyed. The null will make it easier to see if that happens somehow.

> > Source/WebKit/chromium/public/WebFrameClient.h:313
> > +    // Notifies that this frame's script context has been destroyed.
> > +    virtual void didDestroyScriptContext(WebFrame*) { }
> 
> Should we add a comment about removing this function once the roll is complete?

Done.

[WebKit Review Bot]

Comment on attachment 107571 [details]
Patch

Attachment 107571 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9682347

[WebKit Review Bot]

Comment on attachment 107571 [details]
Patch

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 107571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107571&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:301
> +    // FIXME: Remove this when Chromium is updated to use the below version.

WebKit API changes LGTM.  Sorry, I didn't get a chance to review the entire patch...

[Aaron Boodman]

Created attachment 107754 [details]
Patch

[Aaron Boodman]

Latest patch fixes the compile error. abarth?

[Adam Barth]

Comment on attachment 107754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107754&action=review

This looks great.  You might want to check over your indentation in the tests.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Lies!  There are tests!

> Source/WebCore/bindings/v8/V8IsolatedContext.cpp:52
> -    : m_world(IsolatedWorld::create(worldId))
> +    : m_world(IsolatedWorld::create(worldId)), m_frame(proxy->frame())

This should be on two separate lines.

Is there any risk that the V8IsolatedContext could outlive the frame?  If so, we'll need a mechanism to make sure we don't hold a dangling pointer.  V8IsolatedContext's lifetime is managed by the garbage collector, right?  That means it can live arbitrarily long.

> Source/WebCore/bindings/v8/V8IsolatedContext.cpp:84
> +    m_frame->loader()->client()->willReleaseScriptContext(context(), m_world->id());
>      m_context->get().MakeWeak(this, &contextWeakReferenceCallback);
> +    m_frame = 0;

Ah, I see that we clear out the pointer before turning our fortunes over to the garbage collector.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:304
> +    std::vector<Notification*> createNotifications;
> +    std::vector<Notification*> releaseNotifications;

These names are confusing.  They feel like method names (verbs).  Maybe createNotificationsReceived?  logOfCreateNotifications ?

I asume we aren't allowed to depend upon base here to avoid the manual memory management.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:385
> +    for (size_t i = 0; i < webFrameClient.releaseNotifications.size(); ++i) {
> +      EXPECT_TRUE(webFrameClient.releaseNotifications[i]->Equals(
> +          webFrameClient.createNotifications[webFrameClient.createNotifications.size() - 3 - i]));
> +    }

- 3 ?  Looks odd, but ok.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:446
> +    for (size_t i = 0; i < webFrameClient.releaseNotifications.size(); ++i) {
> +      if (webFrameClient.releaseNotifications[i]->Equals(webFrameClient.createNotifications[0]))
> +        ++matchCount;
> +    }

This file is supposed to use four-space indent.

[Adam Barth]

I assume you'll take care of any fancy footwork needed to land the API change safely.

[Aaron Boodman]

Committed r95385: <http://trac.webkit.org/changeset/95385>

[Aaron Boodman]

Patch got reverted: https://bugs.webkit.org/show_bug.cgi?id=68395

[Aaron Boodman]

Created attachment 108037 [details]
Patch

[Aaron Boodman]

In the previously reviewed patch, I accidentally removed the plumbing that made the old APIs work, which broke Chromium tests. See FrameLoaderClientImpl.cpp.

[Adam Barth]

Comment on attachment 108037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108037&action=review

> Source/WebCore/bindings/v8/V8IsolatedContext.cpp:52
> +    : m_world(IsolatedWorld::create(worldId)), m_frame(proxy->frame())

These should be on separate lines.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:294
> +        Notification(WebFrame* frame, v8::Handle<v8::Context> context, int worldId)
> +            : frame(frame) , context(v8::Persistent<v8::Context>::New(context)), worldId(worldId)

This should be on separate lines.

[Aaron Boodman]

Committed r95560: <http://trac.webkit.org/changeset/95560>