Title: [113818] trunk
- Revision
- 113818
- Author
- dgro...@chromium.org
- Date
- 2012-04-10 19:59:54 -0700 (Tue, 10 Apr 2012)
Log Message
Notify observers of WorkerRunLoop stopping before the V8 isolate dies.
https://bugs.webkit.org/show_bug.cgi?id=83104
Source/WebCore:
PlatformSupport::didStopWorkerRunLoop ultimately causes
~V8AbstractEventListener to call
v8::Local<v8::Object>::New(m_listener) after the V8 isolate has been
disposed, which manifests as a crash in V8.
The current code in trunk runs this at shutdown:
1) removeAllDOMObjects()
2) dispose of V8
3) didStopWorkerRunLoop() <-- problem
This patch changes the order to be:
1) removeAllDOMObjects()
2) didStopWorkerRunLoop()
3) dispose of V8
We put didStopWorkerRunLoop after removeAllDOMObjects because we don't
want chromium code that runs on a webcore worker to run after it
receives the didStopWorkerRunLoop signal. The destructors of some IDB
objects are run by removeAllDOMObjects, so putting
didStopWorkerRunLoop before removeAllDOMObjects would violate that
constraint.
It's possible that there's a lower layer fix available in V8 or the
bindings.
Reviewed by David Levin.
Test: storage/indexeddb/pending-version-change-on-exit.html
* bindings/v8/WorkerScriptController.cpp:
(WebCore::WorkerScriptController::~WorkerScriptController):
New location of didStopWorkerRunLoop. removeAllDOMObjects and V8
disposal are called here, to run something between them it also has
to go here.
* workers/WorkerThread.cpp:
(WebCore::WorkerThread::workerThread): Old location of
didStopWorkerRunLoop.
LayoutTests:
Reviewed by David Levin.
* storage/indexeddb/pending-version-change-on-exit-expected.txt: Added.
* storage/indexeddb/pending-version-change-on-exit.html: Added.
* storage/indexeddb/resources/pending-version-change-on-exit.js: Added.
(test.request.onsuccess.request.onblocked):
(test.request.onsuccess):
(test):
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (113817 => 113818)
--- trunk/LayoutTests/ChangeLog 2012-04-11 02:57:47 UTC (rev 113817)
+++ trunk/LayoutTests/ChangeLog 2012-04-11 02:59:54 UTC (rev 113818)
@@ -1,3 +1,17 @@
+2012-04-10 David Grogan <dgro...@chromium.org>
+
+ Notify observers of WorkerRunLoop stopping before the V8 isolate dies.
+ https://bugs.webkit.org/show_bug.cgi?id=83104
+
+ Reviewed by David Levin.
+
+ * storage/indexeddb/pending-version-change-on-exit-expected.txt: Added.
+ * storage/indexeddb/pending-version-change-on-exit.html: Added.
+ * storage/indexeddb/resources/pending-version-change-on-exit.js: Added.
+ (test.request.onsuccess.request.onblocked):
+ (test.request.onsuccess):
+ (test):
+
2012-04-10 Adam Klein <ad...@chromium.org>
Break the association between form controls and their owner when the owner leaves the tree
Added: trunk/LayoutTests/storage/indexeddb/pending-version-change-on-exit-expected.txt (0 => 113818)
--- trunk/LayoutTests/storage/indexeddb/pending-version-change-on-exit-expected.txt (rev 0)
+++ trunk/LayoutTests/storage/indexeddb/pending-version-change-on-exit-expected.txt 2012-04-11 02:59:54 UTC (rev 113818)
@@ -0,0 +1,12 @@
+No crashes when there are event listeners in a worker on shutdown
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
+
+PASS Didn't crash!
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/storage/indexeddb/pending-version-change-on-exit.html (0 => 113818)
--- trunk/LayoutTests/storage/indexeddb/pending-version-change-on-exit.html (rev 0)
+++ trunk/LayoutTests/storage/indexeddb/pending-version-change-on-exit.html 2012-04-11 02:59:54 UTC (rev 113818)
@@ -0,0 +1,42 @@
+<html>
+<head>
+<script src=""
+<script src=""
+</head>
+<body>
+<script>
+
+description("No crashes when there are event listeners in a worker on shutdown");
+
+function test() {
+ removeVendorPrefixes();
+ if (self.location.search == "?second") {
+ testPassed("Didn't crash!");
+ finishJSTest();
+ return;
+ }
+ dbname = "pending-version-change-on-exit";
+ evalAndLog("request = indexedDB.open(\"" + dbname + "\")");
+ request._onsuccess_ = startTheWorker;
+ request._onblocked_ = unexpectedBlockedCallback;
+ request._onerror_ = unexpectedErrorCallback;
+}
+
+function startTheWorker() {
+ var worker = startWorker("resources/pending-version-change-on-exit.js");
+ realFinishJSTest = finishJSTest;
+ worker._onerror_ = function(e) {
+ testFailed(e.message);
+ realFinishJSTest();
+ };
+ finishJSTest = function() {
+ self.location = self.location + "?second";
+ }
+}
+
+test();
+
+</script>
+<script src=""
+</body>
+</html>
Added: trunk/LayoutTests/storage/indexeddb/resources/pending-version-change-on-exit.js (0 => 113818)
--- trunk/LayoutTests/storage/indexeddb/resources/pending-version-change-on-exit.js (rev 0)
+++ trunk/LayoutTests/storage/indexeddb/resources/pending-version-change-on-exit.js 2012-04-11 02:59:54 UTC (rev 113818)
@@ -0,0 +1,21 @@
+if (this.importScripts) {
+ importScripts('../../../fast/js/resources/js-test-pre.js');
+ importScripts('shared.js');
+}
+
+function test() {
+ removeVendorPrefixes();
+ dbname = "pending-version-change-on-exit";
+ evalAndLog("request = indexedDB.open(\"" + dbname + "\")");
+ request._onsuccess_ = function(e) {
+ db = request.result;
+ evalAndLog("request = db.setVersion(1)");
+ request._onsuccess_ = unexpectedSuccessCallback;
+ request._onblocked_ = function() {
+ testPassed("worker received blocked event.");
+ finishJSTest();
+ };
+ };
+}
+
+test();
Modified: trunk/Source/WebCore/ChangeLog (113817 => 113818)
--- trunk/Source/WebCore/ChangeLog 2012-04-11 02:57:47 UTC (rev 113817)
+++ trunk/Source/WebCore/ChangeLog 2012-04-11 02:59:54 UTC (rev 113818)
@@ -1,3 +1,47 @@
+2012-04-10 David Grogan <dgro...@chromium.org>
+
+ Notify observers of WorkerRunLoop stopping before the V8 isolate dies.
+ https://bugs.webkit.org/show_bug.cgi?id=83104
+
+ PlatformSupport::didStopWorkerRunLoop ultimately causes
+ ~V8AbstractEventListener to call
+ v8::Local<v8::Object>::New(m_listener) after the V8 isolate has been
+ disposed, which manifests as a crash in V8.
+
+ The current code in trunk runs this at shutdown:
+ 1) removeAllDOMObjects()
+ 2) dispose of V8
+ 3) didStopWorkerRunLoop() <-- problem
+
+ This patch changes the order to be:
+ 1) removeAllDOMObjects()
+ 2) didStopWorkerRunLoop()
+ 3) dispose of V8
+
+ We put didStopWorkerRunLoop after removeAllDOMObjects because we don't
+ want chromium code that runs on a webcore worker to run after it
+ receives the didStopWorkerRunLoop signal. The destructors of some IDB
+ objects are run by removeAllDOMObjects, so putting
+ didStopWorkerRunLoop before removeAllDOMObjects would violate that
+ constraint.
+
+ It's possible that there's a lower layer fix available in V8 or the
+ bindings.
+
+ Reviewed by David Levin.
+
+ Test: storage/indexeddb/pending-version-change-on-exit.html
+
+ * bindings/v8/WorkerScriptController.cpp:
+ (WebCore::WorkerScriptController::~WorkerScriptController):
+ New location of didStopWorkerRunLoop. removeAllDOMObjects and V8
+ disposal are called here, to run something between them it also has
+ to go here.
+
+ * workers/WorkerThread.cpp:
+ (WebCore::WorkerThread::workerThread): Old location of
+ didStopWorkerRunLoop.
+
2012-04-10 Adam Klein <ad...@chromium.org>
Break the association between form controls and their owner when the owner leaves the tree
Modified: trunk/Source/WebCore/bindings/v8/WorkerScriptController.cpp (113817 => 113818)
--- trunk/Source/WebCore/bindings/v8/WorkerScriptController.cpp 2012-04-11 02:57:47 UTC (rev 113817)
+++ trunk/Source/WebCore/bindings/v8/WorkerScriptController.cpp 2012-04-11 02:59:54 UTC (rev 113818)
@@ -65,6 +65,12 @@
WorkerScriptController::~WorkerScriptController()
{
removeAllDOMObjects();
+#if PLATFORM(CHROMIUM)
+ // The corresponding call to didStartWorkerRunLoop is in
+ // WorkerThread::workerThread().
+ // See http://webkit.org/b/83104#c14 for why this is here.
+ PlatformSupport::didStopWorkerRunLoop(&m_workerContext->thread()->runLoop());
+#endif
m_proxy.clear();
m_isolate->Exit();
V8BindingPerIsolateData::dispose(m_isolate);
Modified: trunk/Source/WebCore/workers/WorkerThread.cpp (113817 => 113818)
--- trunk/Source/WebCore/workers/WorkerThread.cpp 2012-04-11 02:57:47 UTC (rev 113817)
+++ trunk/Source/WebCore/workers/WorkerThread.cpp 2012-04-11 02:59:54 UTC (rev 113818)
@@ -142,6 +142,8 @@
}
}
#if PLATFORM(CHROMIUM)
+ // The corresponding call to didStopWorkerRunLoop is in
+ // ~WorkerScriptController.
PlatformSupport::didStartWorkerRunLoop(&m_runLoop);
#endif
@@ -157,10 +159,6 @@
runEventLoop();
-#if PLATFORM(CHROMIUM)
- PlatformSupport::didStopWorkerRunLoop(&m_runLoop);
-#endif
-
ThreadIdentifier threadID = m_threadID;
ASSERT(m_workerContext->hasOneRef());
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
