Closed Bug 692991 Opened 13 years ago Closed 13 years ago

IndexedDB: IDBDatabase.transaction should throw if the database hasn't been fully opened yet

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: khuey)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch PatchSplinter Review
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.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #568865 - Flags: review?(bent.mozilla)
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.
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 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().
Attachment #568865 - Flags: review?(bent.mozilla) → review+
(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.
Attached patch Patch (obsolete) — Splinter Review
Would you prefer to move the logic into AsyncConnectionHelper like so?
Attachment #568865 - Attachment is obsolete: true
Attachment #569080 - Flags: review?(bent.mozilla)
(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 :-/
Attachment #569080 - Attachment is obsolete: true
Attachment #569080 - Flags: review?(bent.mozilla)
https://hg.mozilla.org/mozilla-central/rev/26ac81280f33
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla10 → ---
Relanded with an additional test fix.

https://hg.mozilla.org/mozilla-central/rev/0b03882d8edf
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla10 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: