Log Message
[chromium] DatabaseObserver needs threadsafe fixes and other clean-up. https://bugs.webkit.org/show_bug.cgi?id=74558
Reviewed by Dmitry Titov. Source/WebKit/chromium: The important part of this fix is the removal of AllowCrossThreadAccess so that ref counting happens appropriately. Minor clean up throughout: Removed unnecessary WTF prefix in many of these places and unnecessary String(). * src/DatabaseObserver.cpp: (WebKit::AllowDatabaseMainThreadBridge::create): (WebKit::AllowDatabaseMainThreadBridge::signalCompleted): Get rid of AllowCrossThreadAccess so that "this" get ref counted and remove mode from being a member variable since a String in a ThreadSafeRefCounted class is bad. (WebKit::AllowDatabaseMainThreadBridge::AllowDatabaseMainThreadBridge): Ditto. (WebKit::AllowDatabaseMainThreadBridge::allowDatabaseTask): Pass through mode and minor code cleaning to do only one call to signalCompleted. (WebKit::allowDatabaseForWorker): Just removed unnecessary String()'s. LayoutTests: Added a test which would fail without some of these fixes. * http/tests/workers/interrupt-database-sync-open-crash.html: Added. * http/tests/workers/resources/open-database-sync.js: Added. (onmessage):
Modified Paths
- trunk/LayoutTests/ChangeLog
- trunk/Source/WebKit/chromium/ChangeLog
- trunk/Source/WebKit/chromium/src/DatabaseObserver.cpp
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (102893 => 102894)
--- trunk/LayoutTests/ChangeLog 2011-12-15 06:48:32 UTC (rev 102893)
+++ trunk/LayoutTests/ChangeLog 2011-12-15 06:51:20 UTC (rev 102894)
@@ -1,3 +1,16 @@
+2011-12-14 David Levin <[email protected]>
+
+ [chromium] DatabaseObserver needs threadsafe fixes and other clean-up.
+ https://bugs.webkit.org/show_bug.cgi?id=74558
+
+ Reviewed by Dmitry Titov.
+
+ Added a test which would fail without some of these fixes.
+
+ * http/tests/workers/interrupt-database-sync-open-crash.html: Added.
+ * http/tests/workers/resources/open-database-sync.js: Added.
+ (onmessage):
+
2011-12-14 Kent Tamura <[email protected]>
[Chromium] Test expectatino update.
Added: trunk/LayoutTests/http/tests/workers/interrupt-database-sync-open-crash.html (0 => 102894)
--- trunk/LayoutTests/http/tests/workers/interrupt-database-sync-open-crash.html (rev 0)
+++ trunk/LayoutTests/http/tests/workers/interrupt-database-sync-open-crash.html 2011-12-15 06:51:20 UTC (rev 102894)
@@ -0,0 +1,76 @@
+<html>
+<head>
+<script src=''></script>
+<script>
+var workersStarted;
+var workersClosed;
+
+// Do the test 10 times because the crash didn't happen every time, but
+// it happen with a high degree of confidence in 10 iterations.
+var remainingIterations = 10;
+// 30 workers seemed to cause the crash to happen frequently.
+var workers = new Array(30);
+
+function nextIteration()
+{
+ log('Remaining iterations: ' + remainingIterations);
+ waitUntilWorkerThreadsExit(startWorkers)
+}
+
+function startWorkers()
+{
+ if (!remainingIterations) {
+ done();
+ return;
+ }
+ remainingIterations--;
+
+ workersStarted = 0;
+ workersClosed = 0;
+ for (var i = 0; i < workers.length; ++i) {
+ workers[i] = new Worker('resources/open-database-sync.js?arg=' + i)
+ workers[i]._onmessage_ = waitForAllWorkersToStart;
+ }
+}
+
+// Do our best to try to interrupt the databse open
+// call by waiting for the worker to start and then
+// telling it to do the open database call (and
+// then terminate the worker).
+function waitForAllWorkersToStart()
+{
+ workersStarted++;
+ if (workersStarted < workers.length)
+ return;
+
+ for (var i = 0; i < workers.length; ++i)
+ workers[i].postMessage('openDatabaseSync');
+
+ setTimeout('closeWorker()', 0);
+}
+
+function closeWorker()
+{
+ workers[workersClosed].terminate();
+ workersClosed++;
+ if (workersClosed < workers.length)
+ setTimeout('closeWorker()', 3);
+ else
+ nextIteration();
+}
+
+function runTest()
+{
+ if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+ }
+ nextIteration();
+}
+</script>
+</head>
+<body _onload_='runTest()'>
+<div id='result'>
+</div>
+</body>
+</html>
Added: trunk/LayoutTests/http/tests/workers/resources/open-database-sync.js (0 => 102894)
--- trunk/LayoutTests/http/tests/workers/resources/open-database-sync.js (rev 0)
+++ trunk/LayoutTests/http/tests/workers/resources/open-database-sync.js 2011-12-15 06:51:20 UTC (rev 102894)
@@ -0,0 +1,7 @@
+postMessage('started');
+
+_onmessage_ = function(evt)
+{
+ if (evt.data == 'openDatabaseSync')
+ openDatabaseSync('', '', '', 1);
+}
Modified: trunk/Source/WebKit/chromium/ChangeLog (102893 => 102894)
--- trunk/Source/WebKit/chromium/ChangeLog 2011-12-15 06:48:32 UTC (rev 102893)
+++ trunk/Source/WebKit/chromium/ChangeLog 2011-12-15 06:51:20 UTC (rev 102894)
@@ -1,3 +1,26 @@
+2011-12-14 David Levin <[email protected]>
+
+ [chromium] DatabaseObserver needs threadsafe fixes and other clean-up.
+ https://bugs.webkit.org/show_bug.cgi?id=74558
+
+ Reviewed by Dmitry Titov.
+
+ The important part of this fix is the removal of AllowCrossThreadAccess so
+ that ref counting happens appropriately.
+
+ Minor clean up throughout: Removed unnecessary WTF prefix in many
+ of these places and unnecessary String().
+
+ * src/DatabaseObserver.cpp:
+ (WebKit::AllowDatabaseMainThreadBridge::create):
+ (WebKit::AllowDatabaseMainThreadBridge::signalCompleted): Get rid of
+ AllowCrossThreadAccess so that "this" get ref counted and remove mode from being
+ a member variable since a String in a ThreadSafeRefCounted class is bad.
+ (WebKit::AllowDatabaseMainThreadBridge::AllowDatabaseMainThreadBridge): Ditto.
+ (WebKit::AllowDatabaseMainThreadBridge::allowDatabaseTask): Pass through mode
+ and minor code cleaning to do only one call to signalCompleted.
+ (WebKit::allowDatabaseForWorker): Just removed unnecessary String()'s.
+
2011-12-14 Tony Chang <[email protected]>
[chromium] Remove redundant third_party entries from chromium DEPS
Modified: trunk/Source/WebKit/chromium/src/DatabaseObserver.cpp (102893 => 102894)
--- trunk/Source/WebKit/chromium/src/DatabaseObserver.cpp 2011-12-15 06:48:32 UTC (rev 102893)
+++ trunk/Source/WebKit/chromium/src/DatabaseObserver.cpp 2011-12-15 06:51:20 UTC (rev 102894)
@@ -64,7 +64,7 @@
// call back to the worker context.
class AllowDatabaseMainThreadBridge : public ThreadSafeRefCounted<AllowDatabaseMainThreadBridge> {
public:
- static PassRefPtr<AllowDatabaseMainThreadBridge> create(WebCore::WorkerLoaderProxy* workerLoaderProxy, const WTF::String& mode, NewWebCommonWorkerClient* commonClient, WebFrame* frame, const WTF::String& name, const WTF::String& displayName, unsigned long estimatedSize)
+ static PassRefPtr<AllowDatabaseMainThreadBridge> create(WebCore::WorkerLoaderProxy* workerLoaderProxy, const String& mode, NewWebCommonWorkerClient* commonClient, WebFrame* frame, const String& name, const String& displayName, unsigned long estimatedSize)
{
return adoptRef(new AllowDatabaseMainThreadBridge(workerLoaderProxy, mode, commonClient, frame, name, displayName, estimatedSize));
}
@@ -82,33 +82,29 @@
}
// This method is invoked on the main thread.
- void signalCompleted(bool result)
+ void signalCompleted(const String& mode, bool result)
{
MutexLocker locker(m_mutex);
- if (m_workerLoaderProxy)
- m_workerLoaderProxy->postTaskForModeToWorkerContext(
- createCallbackTask(&didComplete, WebCore::AllowCrossThreadAccess(this), result),
- m_mode);
+ if (!m_workerLoaderProxy)
+ return;
+ m_workerLoaderProxy->postTaskForModeToWorkerContext(createCallbackTask(&didComplete, this, result), mode);
}
private:
- AllowDatabaseMainThreadBridge(WebCore::WorkerLoaderProxy* workerLoaderProxy, const WTF::String& mode, NewWebCommonWorkerClient* commonClient, WebFrame* frame, const WTF::String& name, const WTF::String& displayName, unsigned long estimatedSize)
+ AllowDatabaseMainThreadBridge(WebCore::WorkerLoaderProxy* workerLoaderProxy, const String& mode, NewWebCommonWorkerClient* commonClient, WebFrame* frame, const String& name, const String& displayName, unsigned long estimatedSize)
: m_workerLoaderProxy(workerLoaderProxy)
- , m_mode(mode)
{
WebWorkerBase::dispatchTaskToMainThread(
- createCallbackTask(&allowDatabaseTask, WebCore::AllowCrossThreadAccess(commonClient),
+ createCallbackTask(&allowDatabaseTask, mode, WebCore::AllowCrossThreadAccess(commonClient),
WebCore::AllowCrossThreadAccess(frame),
- String(name), String(displayName), estimatedSize,
- WebCore::AllowCrossThreadAccess(this)));
+ name, displayName, estimatedSize,
+ this));
}
- static void allowDatabaseTask(WebCore::ScriptExecutionContext* context, NewWebCommonWorkerClient* commonClient, WebFrame* frame, const WTF::String name, const WTF::String displayName, unsigned long estimatedSize, PassRefPtr<AllowDatabaseMainThreadBridge> bridge)
+ static void allowDatabaseTask(WebCore::ScriptExecutionContext* context, const String mode, NewWebCommonWorkerClient* commonClient, WebFrame* frame, const String name, const String displayName, unsigned long estimatedSize, PassRefPtr<AllowDatabaseMainThreadBridge> bridge)
{
- if (commonClient)
- bridge->signalCompleted(commonClient->allowDatabase(frame, name, displayName, estimatedSize));
- else
- bridge->signalCompleted(false);
+ bool allowDatabase = commonClient ? commonClient->allowDatabase(frame, name, displayName, estimatedSize) : false;
+ bridge->signalCompleted(mode, allowDatabase);
}
static void didComplete(WebCore::ScriptExecutionContext* context, PassRefPtr<AllowDatabaseMainThreadBridge> bridge, bool result)
@@ -119,7 +115,6 @@
bool m_result;
Mutex m_mutex;
WebCore::WorkerLoaderProxy* m_workerLoaderProxy;
- WTF::String m_mode;
};
bool allowDatabaseForWorker(NewWebCommonWorkerClient* commonClient, WebFrame* frame, const WebString& name, const WebString& displayName, unsigned long estimatedSize)
@@ -134,7 +129,7 @@
String mode = allowDatabaseMode;
mode.append(String::number(runLoop.createUniqueId()));
- RefPtr<AllowDatabaseMainThreadBridge> bridge = AllowDatabaseMainThreadBridge::create(workerLoaderProxy, mode, commonClient, frame, String(name), String(displayName), estimatedSize);
+ RefPtr<AllowDatabaseMainThreadBridge> bridge = AllowDatabaseMainThreadBridge::create(workerLoaderProxy, mode, commonClient, frame, name, displayName, estimatedSize);
// Either the bridge returns, or the queue gets terminated.
if (runLoop.runInMode(workerContext, mode) == MessageQueueTerminated) {
_______________________________________________ webkit-changes mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
