Bug 67828 - Rework script context creation/release notifications
Summary: Rework script context creation/release notifications
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aaron Boodman
URL:
Keywords:
Depends on: 68395
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-08 19:22 PDT by Aaron Boodman
Modified: 2011-09-20 11:57 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.06 KB, patch)
2011-09-08 19:23 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (22.15 KB, patch)
2011-09-15 17:41 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (22.15 KB, patch)
2011-09-16 17:44 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (23.51 KB, patch)
2011-09-20 11:47 PDT, Aaron Boodman
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2011-09-08 19:22:49 PDT
Rework script context creation/release notifications
Comment 1 Aaron Boodman 2011-09-08 19:23:05 PDT
Created attachment 106832 [details]
Patch
Comment 2 Aaron Boodman 2011-09-08 19:25:43 PDT
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
Comment 3 Aaron Boodman 2011-09-08 19:27:46 PDT
I will the older versions from the webkit api in a follow up change.
Comment 4 Adam Barth 2011-09-09 11:23:46 PDT
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?
Comment 5 Aaron Boodman 2011-09-15 17:41:50 PDT
Created attachment 107571 [details]
Patch
Comment 6 Aaron Boodman 2011-09-15 17:43:04 PDT
(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.
Comment 7 WebKit Review Bot 2011-09-15 18:22:43 PDT
Comment on attachment 107571 [details]
Patch

Attachment 107571 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9682347
Comment 8 WebKit Review Bot 2011-09-15 19:11:00 PDT
Comment on attachment 107571 [details]
Patch

Attachment 107571 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9685307
Comment 9 Darin Fisher (:fishd, Google) 2011-09-15 23:52:31 PDT
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...
Comment 10 Aaron Boodman 2011-09-16 17:44:41 PDT
Created attachment 107754 [details]
Patch
Comment 11 Aaron Boodman 2011-09-16 17:45:06 PDT
Latest patch fixes the compile error. abarth?
Comment 12 Adam Barth 2011-09-16 17:56:07 PDT
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.
Comment 13 Adam Barth 2011-09-16 17:56:32 PDT
I assume you'll take care of any fancy footwork needed to land the API change safely.
Comment 14 Aaron Boodman 2011-09-17 15:46:25 PDT
Committed r95385: <http://trac.webkit.org/changeset/95385>
Comment 15 Aaron Boodman 2011-09-20 11:38:25 PDT
Patch got reverted: https://bugs.webkit.org/show_bug.cgi?id=68395
Comment 16 Aaron Boodman 2011-09-20 11:47:21 PDT
Created attachment 108037 [details]
Patch
Comment 17 Aaron Boodman 2011-09-20 11:49:17 PDT
In the previously reviewed patch, I accidentally removed the plumbing that made the old APIs work, which broke Chromium tests. See FrameLoaderClientImpl.cpp.
Comment 18 Adam Barth 2011-09-20 11:50:27 PDT
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.
Comment 19 Aaron Boodman 2011-09-20 11:57:25 PDT
Committed r95560: <http://trac.webkit.org/changeset/95560>