[v8] Error in getScriptExecutionContext() when worker context is terminating
Created attachment 51567 [details] Patch
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".
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.
Created attachment 51651 [details] Patch
Attachment 51651 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1207043
Created attachment 51653 [details] Patch
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.
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. :)
(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;
Comment on attachment 51653 [details] Patch Clearing flags on attachment: 51653 Committed r56580: <http://trac.webkit.org/changeset/56580>
All reviewed patches have been landed. Closing bug.