Reviewed by Darin.
authorap@webkit.org <ap@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2008 12:51:41 +0000 (12:51 +0000)
committerap@webkit.org <ap@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2008 12:51:41 +0000 (12:51 +0000)
        <rdar://problem/5740042> Database termination issues

        Test: storage/close-during-stress-test.html

        * dom/Document.cpp:
        (WebCore::Document::databaseThread):
        * dom/Document.h:
        Don't re-create the database thread if it has been already terminated.

        * storage/Database.h: (WebCore::Database::document): Changed m_database to a RefPtr to avoid
        having a hanging reference.

        * storage/DatabaseThread.cpp:
        (WebCore::DatabaseThread::requestTermination):

        * storage/SQLTransaction.cpp: (WebCore::SQLTransaction::~SQLTransaction): Removed logging.
        Transactions are deleted during GC, so it's usually not importatnt to know when it happens.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@30195 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/storage/close-during-stress-test-expected.txt [new file with mode: 0644]
LayoutTests/storage/close-during-stress-test.html [new file with mode: 0644]
LayoutTests/storage/resources/stress-frame.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/dom/Document.cpp
WebCore/dom/Document.h
WebCore/storage/Database.h
WebCore/storage/DatabaseThread.cpp
WebCore/storage/SQLTransaction.cpp

index ae272d3..68371dc 100644 (file)
@@ -1,3 +1,14 @@
+2008-02-13  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        <rdar://problem/5740042> Database termination issues
+
+        * storage/close-during-stress-test-expected.txt: Added.
+        * storage/close-during-stress-test.html: Added.
+        * storage/resources: Added.
+        * storage/resources/stress-frame.html: Added.
+
 2008-02-13  Darin Adler  <darin@apple.com>
 
         - check in results for these tests
diff --git a/LayoutTests/storage/close-during-stress-test-expected.txt b/LayoutTests/storage/close-during-stress-test-expected.txt
new file mode 100644 (file)
index 0000000..2370b9b
--- /dev/null
@@ -0,0 +1,6 @@
+CONSOLE MESSAGE: line 37: Can't find variable: loadNotes
+Should not crash or cause an assertion failure.
+
+A JavaScript failure on the console is expected, however, as the global object is cleared when closing a frame. It actually helps to cause database activity by throwing an exception from a callback.
+
+
diff --git a/LayoutTests/storage/close-during-stress-test.html b/LayoutTests/storage/close-during-stress-test.html
new file mode 100644 (file)
index 0000000..ff105c4
--- /dev/null
@@ -0,0 +1,18 @@
+<body>
+<p>Should not crash or cause an assertion failure.</p>
+<p>A JavaScript failure on the console is expected, however, as the global object is cleared when closing a frame.
+It actually helps to cause database activity by throwing an exception from a callback.</p>
+<iframe src="resources/stress-frame.html" onload="startTest()"></iframe>
+<script>
+if (window.layoutTestController) {
+    layoutTestController.waitUntilDone();
+    layoutTestController.dumpAsText();
+}
+
+function startTest() {
+    setTimeout("document.getElementsByTagName('iframe')[0].src = 'about:blank'", 100);
+    if (window.layoutTestController)
+        setTimeout("layoutTestController.notifyDone()", 500);
+}
+</script>
+</body>
diff --git a/LayoutTests/storage/resources/stress-frame.html b/LayoutTests/storage/resources/stress-frame.html
new file mode 100644 (file)
index 0000000..b99af3c
--- /dev/null
@@ -0,0 +1,51 @@
+<!doctype html>
+<html>
+<head>
+<script>
+var db;
+
+try {
+    if (window.openDatabase) {
+        db = openDatabase("StressTest2", "1.0", "Database stress test", 200000);
+        if (!db)
+            alert("Failed to open the database on disk.  This is probably because the version was bad or there is not enough space left in this domain's quota");
+    } else
+        alert("Couldn't open the database.  Please try with a WebKit nightly with this feature enabled");
+} catch(err) { }
+
+function loaded()
+{
+    db.transaction(function(tx) {
+        tx.executeSql("SELECT COUNT(*) FROM WebkitStickyNotes", [], function(result) {
+            loadNotes();
+        }, function(tx, error) {
+            tx.executeSql("CREATE TABLE WebKitStickyNotes (id REAL UNIQUE, note TEXT)", [], function(result) { 
+                tx.executeSql("INSERT INTO WebKitStickyNotes (id, note) VALUES (?, ?)", [1, 'Text'], function(result) { 
+                    tx.executeSql("INSERT INTO WebKitStickyNotes (id, note) VALUES (?, ?)", [2, 'More Text'], function(result) { 
+                        loadNotes(); 
+                    });
+                });
+            });
+        });
+    });
+}
+
+function loadNotes()
+{
+    db.transaction(function(tx) {
+        tx.executeSql("SELECT id, note FROM WebKitStickyNotes", [], function(tx, result) {
+            loadNotes();
+        }, function(tx, error) {
+            alert('Failed to retrieve notes from database - ' + error.message);
+            return;
+        });
+    });
+}
+
+addEventListener('load', loaded, false);
+</script>
+</head>
+<body>
+<p>This test needs to run without crashes and assertion failures for a while.<p>
+</body>
+</html>
index bfff753..68469e1 100644 (file)
@@ -1,3 +1,25 @@
+2008-02-13  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        <rdar://problem/5740042> Database termination issues
+
+        Test: storage/close-during-stress-test.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::databaseThread):
+        * dom/Document.h:
+        Don't re-create the database thread if it has been already terminated.
+
+        * storage/Database.h: (WebCore::Database::document): Changed m_database to a RefPtr to avoid
+        having a hanging reference.
+
+        * storage/DatabaseThread.cpp:
+        (WebCore::DatabaseThread::requestTermination):
+
+        * storage/SQLTransaction.cpp: (WebCore::SQLTransaction::~SQLTransaction): Removed logging.
+        Transactions are deleted during GC, so it's usually not importatnt to know when it happens.
+
 2008-02-12  Bernhard Rosenkraenzer  <bero@arklinux.org>
 
         Reviewed by Darin.
index 50e9666..50a2933 100644 (file)
@@ -3777,7 +3777,9 @@ void Document::updateFocusAppearanceTimerFired(Timer<Document>*)
 #if ENABLE(DATABASE)
 DatabaseThread* Document::databaseThread()
 {
-    if (!m_databaseThread) {
+    if (!m_databaseThread && !m_hasOpenDatabases) {
+        // Create the database thread on first request - but not if at least one database was already opened,
+        // because in that case we already had a database thread and terminated it and should not create another.
         m_databaseThread = new DatabaseThread(this);
         if (!m_databaseThread->start())
             m_databaseThread = 0;
index 9cac761..fdcb6de 100644 (file)
@@ -874,7 +874,7 @@ public:
     bool processingLoadEvent() const { return m_processingLoadEvent; }
 
 #if ENABLE(DATABASE)
-    DatabaseThread* databaseThread();
+    DatabaseThread* databaseThread();   // Creates the thread as needed, but not if it has been already terminated.
     void setHasOpenDatabases() { m_hasOpenDatabases = true; }
     bool hasOpenDatabases() { return m_hasOpenDatabases; }
 #endif
@@ -943,7 +943,7 @@ private:
 
 #if ENABLE(DATABASE)
     RefPtr<DatabaseThread> m_databaseThread;
-    bool m_hasOpenDatabases;
+    bool m_hasOpenDatabases;    // This never changes back to false, even as the database thread is closed.
 #endif
 #if USE(LOW_BANDWIDTH_DISPLAY)
     bool m_inLowBandwidthDisplay;
index d562222..426484c 100644 (file)
@@ -81,7 +81,7 @@ public:
 
     Vector<String> tableNames();
 
-    Document* document() const { return m_document; }
+    Document* document() const { return m_document.get(); }
     PassRefPtr<SecurityOrigin> securityOriginCopy() const;
     String stringIdentifier() const;
     
@@ -121,7 +121,7 @@ private:
 
     static void deliverPendingCallback(void*);
 
-    Document* m_document;
+    RefPtr<Document> m_document;
     RefPtr<SecurityOrigin> m_securityOrigin;
     String m_name;
     int m_guid;
index e5b663c..a040bd9 100644 (file)
@@ -58,7 +58,7 @@ bool DatabaseThread::start()
 
 void DatabaseThread::requestTermination()
 {
-    LOG(StorageAPI, "Document owning DatabaseThread %p is going away - starting thread shutdown", this);
+    LOG(StorageAPI, "DatabaseThread %p was asked to terminate\n", this);
     m_queue.kill();
 }
 
index ab62b41..9605a29 100644 (file)
@@ -74,7 +74,6 @@ SQLTransaction::SQLTransaction(Database* db, PassRefPtr<SQLTransactionCallback>
 
 SQLTransaction::~SQLTransaction()
 {
-    LOG(StorageAPI, "Transaction %p destructor\n", this);
 }
 
 void SQLTransaction::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, PassRefPtr<SQLStatementCallback> callback, PassRefPtr<SQLStatementErrorCallback> callbackError, ExceptionCode& e)