Summary: | Rework script context creation/release notifications | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Boodman <aa> | ||||||||||
Component: | WebKit API | Assignee: | Aaron Boodman <aa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 68395 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Aaron Boodman
2011-09-08 19:22:49 PDT
Created attachment 106832 [details]
Patch
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 I will the older versions from the webkit api in a follow up change. 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? Created attachment 107571 [details]
Patch
(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 on attachment 107571 [details] Patch Attachment 107571 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9682347 Comment on attachment 107571 [details] Patch Attachment 107571 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9685307 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... Created attachment 107754 [details]
Patch
Latest patch fixes the compile error. abarth? 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. I assume you'll take care of any fancy footwork needed to land the API change safely. Committed r95385: <http://trac.webkit.org/changeset/95385> Patch got reverted: https://bugs.webkit.org/show_bug.cgi?id=68395 Created attachment 108037 [details]
Patch
In the previously reviewed patch, I accidentally removed the plumbing that made the old APIs work, which broke Chromium tests. See FrameLoaderClientImpl.cpp. 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. Committed r95560: <http://trac.webkit.org/changeset/95560> |