Last Comment Bug 692991 - IndexedDB: IDBDatabase.transaction should throw if the database hasn't been fully opened yet
: IndexedDB: IDBDatabase.transaction should throw if the database hasn't been f...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
: Hsin-Yi Tsai [:hsinyi]
Mentors:
Depends on: 697177
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-10-07 16:21 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2012-03-22 11:52 PDT (History)
4 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (48.17 KB, patch)
2011-10-22 06:42 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch (59.02 KB, patch)
2011-10-24 09:03 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-07 16:21:33 PDT
I.e. if we haven't fired the "success" event on the IDBOpenDBRequest yet.

The reason is that if the upgradeneeded transaction hasn't been committed yet, shit will hit the fan if the transaction is aborted and there are pending transactions on the database already.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-22 06:42:54 PDT
Created attachment 568865 [details] [diff] [review]
Patch

This basically fell out of another patch I was writing, so I'll put the whole thing here.

This rips out the logic in the transaction thread pool that deals with VERSION_CHANGE transactions and leaves that responsibility to the IndexedDatabaseManager.  Then we fix everything up and add some tests.
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-22 12:33:05 PDT
Hmm.. sticking it on the database info seems a bit risky, though it might work.

Just want to make sure that the following works:

req1 = indexeddb.open("foo", 1);
req1.onupgradeneeded = function() {
  req1.result.createObjectStore("foo");
}
req1.onsuccess = function() {
  db1 = req1.result;
  req2 = indexeddb.open("foo", 2);
  db1.transaction("foo");  // This must not fail
}

I.e. even though another instance is trying to upgrade the database, we must still allow already-open database connections to start transactions.

I suspect this will work fine though since req2 can never start running the VERSION_CHANGE transaction until db1 is closed in the example above. Just wanted to confirm.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-22 12:42:21 PDT
The test I added, test_setVersion_exclusion.html, tests exactly that.  The runningVersionChange flag is set to true when the SetVersionHelper is dispatched to the transaction pool (this happens as soon as all DBs are closed) and is set to false again just before the success/error events fire on the request.
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-10-23 09:10:38 PDT
Comment on attachment 568865 [details] [diff] [review]
Patch

Review of attachment 568865 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/AsyncConnectionHelper.h
@@ +124,5 @@
>    }
>  
>    static IDBTransaction* GetCurrentTransaction();
>  
> +  bool HasTransaction()

You don't need this, do you? See below.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +1319,5 @@
> +  nsRefPtr<SetVersionRunnable> runnable;
> +  runnable.swap(mRunnable);
> +
> +  // If the helper has a transaction, dispatch it to the transaction
> +  // threadpool.

Is this ever going to be true? I'd remove the code for this case. Then you can remove the HasTransaction() function.

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +235,5 @@
> +  class WaitForTransactionsToFinishRunnable : public nsIRunnable
> +  {
> +  public:
> +    WaitForTransactionsToFinishRunnable(SetVersionRunnable* aRunnable)
> +      : mRunnable(aRunnable)

Nit: line the : up with 'Wait...'

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +875,5 @@
>    return NS_DispatchToCurrentThread(this);
>  }
>  
>  void
> +OpenDatabaseHelper::BlockDatabase()

I don't think you really need this extra method... Just call EnterSetVersionTransaction() from Init().
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-23 14:18:05 PDT
(In reply to ben turner [:bent] from comment #4)
> Comment on attachment 568865 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> Review of attachment 568865 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/AsyncConnectionHelper.h
> @@ +124,5 @@
> >    }
> >  
> >    static IDBTransaction* GetCurrentTransaction();
> >  
> > +  bool HasTransaction()
> 
> You don't need this, do you? See below.
> 
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +1319,5 @@
> > +  nsRefPtr<SetVersionRunnable> runnable;
> > +  runnable.swap(mRunnable);
> > +
> > +  // If the helper has a transaction, dispatch it to the transaction
> > +  // threadpool.
> 
> Is this ever going to be true? I'd remove the code for this case. Then you
> can remove the HasTransaction() function.

It's always going to be true for now (SetVersionHelper has a transaction).  It will be false in the DeleteDatabase case.  We can talk about this on IRC tomorrow if you like.

> ::: dom/indexedDB/IndexedDatabaseManager.h
> @@ +235,5 @@
> > +  class WaitForTransactionsToFinishRunnable : public nsIRunnable
> > +  {
> > +  public:
> > +    WaitForTransactionsToFinishRunnable(SetVersionRunnable* aRunnable)
> > +      : mRunnable(aRunnable)
> 
> Nit: line the : up with 'Wait...'

Done.

> ::: dom/indexedDB/OpenDatabaseHelper.cpp
> @@ +875,5 @@
> >    return NS_DispatchToCurrentThread(this);
> >  }
> >  
> >  void
> > +OpenDatabaseHelper::BlockDatabase()
> 
> I don't think you really need this extra method... Just call
> EnterSetVersionTransaction() from Init().

Done.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-24 09:03:48 PDT
Created attachment 569080 [details] [diff] [review]
Patch

Would you prefer to move the logic into AsyncConnectionHelper like so?
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-10-24 11:01:02 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> Would you prefer to move the logic into AsyncConnectionHelper like so?

No, I liked it better before :-/
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-24 13:03:20 PDT
https://hg.mozilla.org/mozilla-central/rev/26ac81280f33
Comment 9 Ed Morley [:emorley] 2011-10-24 16:12:19 PDT
Backed out for Moth orange on all platforms & M2 orange on win opt:
https://tbpl.mozilla.org/?rev=26ac81280f33

https://tbpl.mozilla.org/php/getParsedLog.php?id=7010146&tree=Firefox
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/indexedDB/test/browser_quotaPromptAllow.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/indexedDB/test/browser_quotaPromptAllow.js | Found a tab after previous test timed out: http://test1.example.org/browser/dom/indexedDB/test/browser_quotaPrompt.html?v=1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/indexedDB/test/browser_quotaPromptDeny.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/indexedDB/test/browser_quotaPromptDeny.js | Found a tab after previous test timed out: http://test2.example.org/browser/dom/indexedDB/test/browser_quotaPrompt.html?v=5 
}

https://tbpl.mozilla.org/php/getParsedLog.php?id=7009729&tree=Firefox
{
3023 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_setVersion_exclusion.html | This test left crash dumps behind, but we weren't expecting it to!
TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_success_events_after_abort.html | Exited with code -2147483645 during test run
PROCESS-CRASH | /tests/dom/indexedDB/test/test_success_events_after_abort.html | application crashed (minidump found) 
}

https://hg.mozilla.org/mozilla-central/rev/26ac81280f33

(When relanding, the bug number in the commit message will need correcting, as it was listed as 692911)

Sorry!
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-25 05:51:25 PDT
Relanded with an additional test fix.

https://hg.mozilla.org/mozilla-central/rev/0b03882d8edf

Note You need to log in before you can comment on or make changes to this bug.