Summary: | [v8] Error in getScriptExecutionContext() when worker context is terminating | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrew Wilson <atwilson> | ||||||||
Component: | New Bugs | Assignee: | Andrew Wilson <atwilson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dglazkov, dimich, eric, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 107050 | ||||||||||
Attachments: |
|
Description
Andrew Wilson
2010-03-24 17:23:35 PDT
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. |