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)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Assigned: khuey)
References
Details
Attachments
(1 file, 1 obsolete file)
48.17 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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.
Reporter | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
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+
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Assignee | ||
Comment 6•13 years ago
|
||
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 :-/
Assignee | ||
Updated•13 years ago
|
Attachment #569080 -
Attachment is obsolete: true
Attachment #569080 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•13 years ago
|
Attachment #568865 -
Attachment is obsolete: false
Assignee | ||
Comment 8•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/26ac81280f33
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Comment 9•13 years ago
|
||
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 → ---
Assignee | ||
Comment 10•13 years ago
|
||
Relanded with an additional test fix. https://hg.mozilla.org/mozilla-central/rev/0b03882d8edf
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Assignee | ||
Updated•12 years ago
|
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.
Description
•