Bug 111839 – [V8] Drop ScriptController::existingWindowShellWorkaroundWorld.

• Component: New Bugs
• Status: RESOLVED INVALID

[Mike West]

[V8] Drop one of the two calls to ScriptController::existingWindowShellWorkaroundWorld.

[Mike West]

Created attachment 192190 [details]
Patch

[Mike West]

From running tests locally, it looks like Dan has already fixed this bit of the problem. Let's see if the bots agree.

[Kentaro Hara]

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? :)

[Mike West]

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.

[Dan Carney]

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

[Mike West]

Created attachment 192203 [details]
Patch

[Mike West]

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

[Dan Carney]

> 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

[Mike West]

Created attachment 192207 [details]
Patch

[Mike West]

+pfeldman, yurys to evaluate my vague understanding of the Inspector bits. :)

[Adam Barth]

Wow this patch is really cool.

[Build Bot]

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

[Build Bot]

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

[Mike West]

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

[Ryosuke Niwa]

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

[Mike West]

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

[Mike West]

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

[Mike West]

Created attachment 192345 [details]
Patch

[Mike West]

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

[Yury Semikhatsky]

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

[Mike West]

(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 on attachment 192345 [details]
Patch

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

go for it

[Mike West]

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

[WebKit Review Bot]

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

[Dan Carney]

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

[Mike West]

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

[Mike West]

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)

[Mike West]

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)