Bug 36565 – [v8] Error in getScriptExecutionContext() when worker context is terminating

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andrew Wilson]

[v8] Error in getScriptExecutionContext() when worker context is terminating

[Andrew Wilson]

Created attachment 51567 [details]
Patch

[Andrew Wilson]

Upstream version of http://code.google.com/p/chromium/issues/detail?id=38970

While running the SharedWorkerHttpAuth test under valgrind, I got the following failed assertion:

Command: 
/Volumes/source/chrome.git/src/xcodebuild/Debug/Chromium.app/Contents/Versions/5.0.360
.0/Chromium\ Helper.app/Contents/MacOS/Chromium\ Helper --type=worker --
channel=58049.0x2040a4d0.1081383255 --enable-logging
Invalid write of size 4
  WebCore::V8Proxy::retrieveWindow(v8::Handle<v8::Context>) 
(/Volumes/source/chrome.git/src/third_party/WebKit/WebCore/WebCore.gyp/../bindings/v8/V8
Proxy.cpp:537)
  WebCore::V8Proxy::retrieveFrame(v8::Handle<v8::Context>) 
(/Volumes/source/chrome.git/src/third_party/WebKit/WebCore/WebCore.gyp/../bindings/v8/V8
Proxy.cpp:543)
  WebCore::V8Proxy::retrieveFrameForCurrentContext() 
(/Volumes/source/chrome.git/src/third_party/WebKit/WebCore/WebCore.gyp/../bindings/v8/V8
Proxy.cpp:566)
  WebCore::getScriptExecutionContext(WebCore::ScriptState*) 
(/Volumes/source/chrome.git/src/third_party/WebKit/WebCore/WebCore.gyp/../bindings/v8/V8
Utilities.cpp:141)
  WebCore::reportException(WebCore::ScriptState*, v8::TryCatch&) 
(/Volumes/source/chrome.git/src/third_party/WebKit/WebCore/WebCore.gyp/../bindings/v8/V8
Utilities.cpp:171)
  WebCore::V8AbstractEventListener::invokeEventHandler(WebCore::ScriptExecutionContext*, 
WebCore::Event*, v8::Handle<v8::Value>) 
(/Volumes/source/chrome.git/src/third_party/WebKit/WebCore/WebCore.gyp/../bindings/v8/V8
AbstractEventListener.cpp:150)
...

Basically, the problem seems to be that if you're running in a worker thread, but 
WorkerContextExecutionProxy::retrieve() returns null, then getScriptExecutionContext() assumes 
that we're running in document context and a failed assertion ensues.

We need to change the API to allow callers to differentiate between "I'm in a worker context but it's shutting down" from "I'm in a document context".

[Adam Barth]

Drive-by: getControllerForContext and getScriptExecutionContext aren't in WebKit style.  You're not supposed to use the "get" prefix for getters.  Just controllerForContext and scriptExecutionContext.

[Andrew Wilson]

Created attachment 51651 [details]
Patch

[WebKit Review Bot]

Attachment 51651 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1207043

[Andrew Wilson]

Created attachment 51653 [details]
Patch

[Andrew Wilson]

I took Adam's suggestion and renamed getControllerForContext() to
controllerForContext(). There are several uses of getScriptExecutionContext(),
so I'll leave renaming that function for another day.

[Nate Chapin]

Comment on attachment 51653 [details]
Patch

Ok.

Is there any reason for us to leave a shortcut in place for getting the WorkerContextExecutionProxy* with a single call (for the cases where "we're not in a worker context" and "we're shutting down" are handled equivalently)?  My guess is that it would just create confusion, but I'll ask your opinion anyway. :)

[Andrew Wilson]

(In reply to comment #8)
> (From update of attachment 51653 [details])
> Ok.
> 
> Is there any reason for us to leave a shortcut in place for getting the
> WorkerContextExecutionProxy* with a single call (for the cases where "we're not
> in a worker context" and "we're shutting down" are handled equivalently)?  My
> guess is that it would just create confusion, but I'll ask your opinion anyway.
> :)

There's only one place in the code currently where we want to treat those cases the same (WebWorkerClientImpl.cpp, where we already know we are in a worker context), so I don't think we need a convenience function (and the problem with a convenience function is people can use it incorrectly so I'd rather make it explicit:

        WorkerScriptController* controller = WorkerScriptController::controllerForContext();
        WorkerContextExecutionProxy* currentContext = controller ? controller->proxy() : 0;

[WebKit Commit Bot]

Comment on attachment 51653 [details]
Patch

Clearing flags on attachment: 51653

Committed r56580: <http://trac.webkit.org/changeset/56580>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.