Bug 111839 - [V8] Drop ScriptController::existingWindowShellWorkaroundWorld.
Summary: [V8] Drop ScriptController::existingWindowShellWorkaroundWorld.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 111922
  Show dependency treegraph
 
Reported: 2013-03-08 03:19 PST by Mike West
Modified: 2014-12-16 00:40 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.80 KB, patch)
2013-03-08 03:21 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2013-03-08 04:54 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2013-03-08 05:40 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2013-03-09 09:59 PST, Mike West
jochen: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-03-08 03:19:52 PST
[V8] Drop one of the two calls to ScriptController::existingWindowShellWorkaroundWorld.
Comment 1 Mike West 2013-03-08 03:21:07 PST
Created attachment 192190 [details]
Patch
Comment 2 Mike West 2013-03-08 03:22:04 PST
From running tests locally, it looks like Dan has already fixed this bit of the problem. Let's see if the bots agree.
Comment 3 Kentaro Hara 2013-03-08 04:09:09 PST
Comment on attachment 192190 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Step 1 towards getting rid of existingWindowShellWorkaroundWorld: we can
> +        kill the first FIXME without impact. The second is more complicated.

Would you elaborate on this? I'd guess that an isolated world can hit the branch.

And, Step 1 for what? :)
Comment 4 Mike West 2013-03-08 04:51:27 PST
Repurposing this bug for all three FIXMEs, since I think I know what's going on now.

Dan originally added ScriptController::existingWindowShellWorkaroundWorld as a workaround to a bug he uncovered in Inspector. I think he thought it was his fault; it turns out that I'd call it an inspector bug instead.

InspectorPageAgent::didClearWindowObjectInWorld() gets called twice after a reload, both rolling out from FrameLoader::dispatchDidClearWindowObjectInWorld. InspectorController::didClearWindowObjectInWorld calls it once, and the result of navigating the frame calls it once. The code to inject is only cleared in the second case, however. Something like this:

    InspectorPageAgent::reload(``)
    InspectorController::didClearWindowObjectInWorld()
        - InspectorPageAgent::Injecting: `(function(){window.foo = 42;})()`
    InspectorInstrumentation::didCommitLoadImpl()
        InspectorPageAgent::frameNavigated()
            InspectorController::didClearWindowObjectInWorld()
                - InspectorPageAgent::Injecting: ``

Dan's code masked this bug by ensuring that the first call was injecting code into an unused world. An upcoming patch will fix it, but fails a different inspector test. I'll take a look at that one in a moment.
Comment 5 Dan Carney 2013-03-08 04:53:33 PST
> Dan's code masked this bug by ensuring that the first call was injecting code into an unused world. An upcoming patch will fix it, but fails a different inspector test. I'll take a look at that one in a moment.

I didn't mask anything, I just refactored an existing workaround to make it a lot more clear that it was a workaround.

Also, make sure you run the full test suite in debug mode.
Comment 6 Mike West 2013-03-08 04:54:17 PST
Created attachment 192203 [details]
Patch
Comment 7 Mike West 2013-03-08 04:55:53 PST
(In reply to comment #6)
> Created an attachment (id=192203) [details]
> Patch

This patch passes inspector/extensions/extensions-reload.html, but fails inspector/debugger/watch-expressions-preserve-expansion.html. I _think_ the latter needs a rebaseline, but I need to investigate a bit more before doing that.
Comment 8 Dan Carney 2013-03-08 04:57:31 PST
> This patch passes inspector/extensions/extensions-reload.html, but fails inspector/debugger/watch-expressions-preserve-expansion.html. I _think_ the latter needs a rebaseline, but I need to investigate a bit more before doing that.

I remember inspector/extensions/extensions-reload.html being the real blocker
Comment 9 Mike West 2013-03-08 05:40:31 PST
Created attachment 192207 [details]
Patch
Comment 10 Mike West 2013-03-08 05:41:14 PST
+pfeldman, yurys to evaluate my vague understanding of the Inspector bits. :)
Comment 11 Adam Barth 2013-03-08 11:48:22 PST
Wow this patch is really cool.
Comment 12 Build Bot 2013-03-08 23:20:15 PST
Comment on attachment 192207 [details]
Patch

Attachment 192207 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17027551

New failing tests:
inspector/debugger/watch-expressions-preserve-expansion.html
Comment 13 Build Bot 2013-03-09 00:07:32 PST
Comment on attachment 192207 [details]
Patch

Attachment 192207 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17113366

New failing tests:
inspector/debugger/watch-expressions-preserve-expansion.html
Comment 14 Mike West 2013-03-09 00:24:37 PST
(In reply to comment #12)
> (From update of attachment 192207 [details])
> Attachment 192207 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-commit-queue.appspot.com/results/17027551
> 
> New failing tests:
> inspector/debugger/watch-expressions-preserve-expansion.html

Ugh. Right, in retrospect this is obvious: if I rebaseline a test after changing only V8 bindings, of course it's going to fail under JSC. :)

I'll investigate more closely.
Comment 15 Ryosuke Niwa 2013-03-09 00:26:44 PST
(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 192207 [details] [details])
> > Attachment 192207 [details] [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: http://webkit-commit-queue.appspot.com/results/17027551
> > 
> > New failing tests:
> > inspector/debugger/watch-expressions-preserve-expansion.html
> 
> Ugh. Right, in retrospect this is obvious: if I rebaseline a test after changing only V8 bindings, of course it's going to fail under JSC. :)

Do we need a similar fix in JSC?
Comment 16 Mike West 2013-03-09 00:28:37 PST
(In reply to comment #15)

> > Ugh. Right, in retrospect this is obvious: if I rebaseline a test after changing only V8 bindings, of course it's going to fail under JSC. :)
> 
> Do we need a similar fix in JSC?

I don't think so, but I need to investigate further. The JSC test expectation certainly looks less wrong than the V8 expectation. :)
Comment 17 Mike West 2013-03-09 01:22:47 PST
(In reply to comment #16)
> (In reply to comment #15)
> 
> > > Ugh. Right, in retrospect this is obvious: if I rebaseline a test after changing only V8 bindings, of course it's going to fail under JSC. :)
> > 
> > Do we need a similar fix in JSC?
> 
> I don't think so, but I need to investigate further. The JSC test expectation certainly looks less wrong than the V8 expectation. :)

It looks like this is another result of the same problem that's worked around in the other test: V8 resets the global object twice, calling WebInspector.ScriptsPanel._debuggerReset, which resets the watch expressions, causing them to refresh their values. Since there's no context at that point, the evaluation in WebInspector.WatchExpressionsSection.update() fails. Ugh.
Comment 18 Mike West 2013-03-09 09:59:26 PST
Created attachment 192345 [details]
Patch
Comment 19 Mike West 2013-03-09 10:01:45 PST
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > 
> > > > Ugh. Right, in retrospect this is obvious: if I rebaseline a test after changing only V8 bindings, of course it's going to fail under JSC. :)
> > > 
> > > Do we need a similar fix in JSC?
> > 
> > I don't think so, but I need to investigate further. The JSC test expectation certainly looks less wrong than the V8 expectation. :)
> 
> It looks like this is another result of the same problem that's worked around in the other test: V8 resets the global object twice, calling WebInspector.ScriptsPanel._debuggerReset, which resets the watch expressions, causing them to refresh their values. Since there's no context at that point, the evaluation in WebInspector.WatchExpressionsSection.update() fails. Ugh.

I banged my head against this for about two hours today; I give up. I need some help from inspector-folk. :)

Since this patch doesn't make that bug any worse, and makes the bindings significantly cleaner, I've simply added the expectation in platform/chromium. Bug 111922 will cover the 2x global-object clearing.
Comment 20 Yury Semikhatsky 2013-03-10 23:51:38 PDT
(In reply to comment #17)
> 
> It looks like this is another result of the same problem that's worked around in the other test: V8 resets the global object twice, calling WebInspector.ScriptsPanel._debuggerReset, which resets the watch expressions, causing them to refresh their values. Since there's no context at that point, the evaluation in WebInspector.WatchExpressionsSection.update() fails. Ugh.

Ho come there is no context at the point where didClearWindowObjectInWorld is dispatched? We count on this event being dispatched as soon as global object is ready.
Comment 21 Mike West 2013-03-11 02:02:58 PDT
(In reply to comment #20)
> (In reply to comment #17)
> > 
> > It looks like this is another result of the same problem that's worked around in the other test: V8 resets the global object twice, calling WebInspector.ScriptsPanel._debuggerReset, which resets the watch expressions, causing them to refresh their values. Since there's no context at that point, the evaluation in WebInspector.WatchExpressionsSection.update() fails. Ugh.
> 
> Ho come there is no context at the point where didClearWindowObjectInWorld is dispatched? We count on this event being dispatched as soon as global object is ready.

That was poorly phrased: there's not _no_ context, the context is simply uninitalized. I don't know enough about either the lifecycle of the context or the Inspector code in question to determine why that's the case. That's kinda what I'm hoping you can help me figure out in bug 111922. :)
Comment 22 jochen 2013-03-12 08:05:38 PDT
Comment on attachment 192345 [details]
Patch

Looks better than what we have right now, compiles & tests pass

go for it
Comment 23 Mike West 2013-03-12 08:39:27 PDT
(In reply to comment #22)
> (From update of attachment 192345 [details])
> Looks better than what we have right now, compiles & tests pass
> 
> go for it

I spoke with Pavel briefly this morning, and he came to more or less the same conclusion.

I don't understand the root cause, and I'll need some help to get there; I'll follow up on that in bug 111922.
Comment 24 WebKit Review Bot 2013-03-12 08:43:44 PDT
Comment on attachment 192345 [details]
Patch

Rejecting attachment 192345 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'validate-changelog', '--non-interactive', 192345, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17010808
Comment 25 Dan Carney 2013-03-12 08:55:03 PDT
> I don't understand the root cause, and I'll need some help to get there; I'll follow up on that in bug 111922.

I'm hoping that when we switch to lazy main world context init, the problem will go away.
Comment 26 Mike West 2013-03-12 09:10:04 PDT
(In reply to comment #25)
> > I don't understand the root cause, and I'll need some help to get there; I'll follow up on that in bug 111922.
> 
> I'm hoping that when we switch to lazy main world context init, the problem will go away.

Of course, now I decide to run some more tests in debug; crashes everywhere! Hooray! r-. :(

The ASSERT at http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/InspectorFrontendClientImpl.cpp#L69 blows up.

Honestly, I have no idea how it's working now. :) I guess we're either not creating a frontend client for the main world, or creating it for the fake world and then throwing it away?

Ugh.
Comment 27 Mike West 2013-03-12 09:11:24 PDT
So I can find it later, here's the stack from the crash:

STDERR: ASSERTION FAILED: !m_frontendHost
STDERR: ../../Source/WebKit/chromium/src/InspectorFrontendClientImpl.cpp(69) : virtual void WebKit::InspectorFrontendClientImpl::windowObjectCleared()
STDERR: 1   0x1228e7c WebKit::InspectorFrontendClientImpl::windowObjectCleared()
STDERR: 2   0x2d31981 WebCore::InspectorController::didClearWindowObjectInWorld(WebCore::Frame*, WebCore::DOMWrapperWorld*)
STDERR: 3   0x2f32892 WebCore::FrameLoader::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld*)
STDERR: 4   0x2f24187 WebCore::FrameLoader::dispatchDidClearWindowObjectsInAllWorlds()
STDERR: 5   0x2f23cf6 WebCore::FrameLoader::receivedFirstData()
STDERR: 6   0x2ee7db2 WebCore::DocumentLoader::commitData(char const*, unsigned long)
STDERR: 7   0x12a38c5 WebKit::WebFrameImpl::commitDocumentData(char const*, unsigned long)
STDERR: 8   0x120fca7 WebKit::FrameLoaderClientImpl::committedLoad(WebCore::DocumentLoader*, char const*, int)
STDERR: 9   0x2ee7fec WebCore::DocumentLoader::commitLoad(char const*, int)
STDERR: 10  0x2ee88bd WebCore::DocumentLoader::receivedData(char const*, int)
STDERR: 11  0x2f64825 WebCore::MainResourceLoader::dataReceived(WebCore::CachedResource*, char const*, int)
STDERR: 12  0x2fd2c81 WebCore::CachedRawResource::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool)
STDERR: 13  0x2f944fd WebCore::SubresourceLoader::sendDataToResource(char const*, int)
STDERR: 14  0x2f94857 WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, bool)
STDERR: 15  0x2f8db37 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
STDERR: 16  0x2378b62 WebCore::ResourceHandleInternal::didReceiveData(WebKit::WebURLLoader*, char const*, int, int)
Comment 28 Mike West 2013-03-12 09:35:44 PDT
And here's the trace if I ASSERT(false) in the initial call to the function. Now I'm debugging with fire.

STDERR: ASSERTION FAILED: false
STDERR: ../../Source/WebKit/chromium/src/InspectorFrontendClientImpl.cpp(65) : virtual void WebKit::InspectorFrontendClientImpl::windowObjectCleared()
STDERR: 1   0x12cdcac WebKit::InspectorFrontendClientImpl::windowObjectCleared()
STDERR: 2   0x2dd6931 WebCore::InspectorController::didClearWindowObjectInWorld(WebCore::Frame*, WebCore::DOMWrapperWorld*)
STDERR: 3   0x2fd7842 WebCore::FrameLoader::dispatchDidClearWindowObjectInWorld(WebCore::DOMWrapperWorld*)
STDERR: 4   0x2782bfe WebCore::ScriptController::windowShell(WebCore::DOMWrapperWorld*)
STDERR: 5   0x27828d9 WebCore::ScriptController::initializeMainWorld()
STDERR: 6   0x2785c65 WebCore::ScriptController::updateDocument()
STDERR: 7   0x316cd3b WebCore::Frame::setDocument(WTF::PassRefPtr<WebCore::Document>)
STDERR: 8   0x2fb34c6 WebCore::DocumentWriter::begin(WebCore::KURL const&, bool, WebCore::Document*)
STDERR: 9   0x2f8cc6f WebCore::DocumentLoader::commitData(char const*, unsigned long)
STDERR: 10  0x1348875 WebKit::WebFrameImpl::commitDocumentData(char const*, unsigned long)
STDERR: 11  0x12b4bb7 WebKit::FrameLoaderClientImpl::committedLoad(WebCore::DocumentLoader*, char const*, int)
STDERR: 12  0x2f8cf9c WebCore::DocumentLoader::commitLoad(char const*, int)
STDERR: 13  0x2f8d86d WebCore::DocumentLoader::receivedData(char const*, int)
STDERR: 14  0x30097d5 WebCore::MainResourceLoader::dataReceived(WebCore::CachedResource*, char const*, int)
STDERR: 15  0x3077c31 WebCore::CachedRawResource::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool)
STDERR: 16  0x30394ad WebCore::SubresourceLoader::sendDataToResource(char const*, int)
STDERR: 17  0x3039807 WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, bool)
STDERR: 18  0x3032ae7 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
STDERR: 19  0x241db12 WebCore::ResourceHandleInternal::didReceiveData(WebKit::WebURLLoader*, char const*, int, int)