Closed
Bug 1151017
Opened 10 years ago
Closed 8 years ago
Support the "close" event on databases
Categories
(Core :: Storage: IndexedDB, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: bent.mozilla, Assigned: bevis, Mentored)
References
()
Details
(Keywords: dev-doc-complete, Whiteboard: [tw-dom] btpp-active)
Attachments
(1 file, 2 obsolete files)
30.77 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Basically if the user agent invalidates the db and the user never called close() specifically then we should fire a "close" event on the db to let the user know what happened.
Whiteboard: [tw-dom]
Assignee | ||
Comment 1•9 years ago
|
||
I'd like to take this bug to be more familiar with IndexedDB-related implementation.
Assignee: nobody → btseng
Mentor: khuey
Assignee | ||
Comment 2•9 years ago
|
||
Specced as [1], in 3.1.1.1 Database Connection, " A connection may be closed by a user agent in exceptional circumstances, for example due to loss of access to the file system or a permission change. If this occurs the user agent MUST run the steps for closing a database connection with the connection and with the forced flag set. " [1] https://w3c.github.io/IndexedDB/#database-connection
And inside Gecko, that corresponds to an Invalidate call on the database.
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Assignee | ||
Comment 4•8 years ago
|
||
1. Add QuotaClient::InvalidateDatabaseHelper to invalidate the databases according to the information provided from QuotaManager. 2. Report an error earlier for the IDBRequest received after database is invalidated to prevent further assertion failure in ConnectionPool::Dispatch(). 3. Trigger IDBDatabase.onclose() if invalidated from user agent. Treeherder result for reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2851b1f42605
Comment 5•8 years ago
|
||
Comment on attachment 8761044 [details] [diff] [review] (v1) Patch: Support the 'close' Event on Databases. Review of attachment 8761044 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsParent.cpp @@ +17311,5 @@ > mgr->InvalidateFileManagers(aPersistenceType, aOrigin); > + > + const nsCString origin(aOrigin); > + > + nsCOMPtr<nsIRunnable> runnable = new InvalidateDatabaseHelper( Do you think it's safe to touch gLiveDatabaseHashtable on this thread ?
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Jan Varga [:janv] from comment #5) > Comment on attachment 8761044 [details] [diff] [review] > (v1) Patch: Support the 'close' Event on Databases. > > Review of attachment 8761044 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/ActorsParent.cpp > @@ +17311,5 @@ > > mgr->InvalidateFileManagers(aPersistenceType, aOrigin); > > + > > + const nsCString origin(aOrigin); > > + > > + nsCOMPtr<nsIRunnable> runnable = new InvalidateDatabaseHelper( > > Do you think it's safe to touch gLiveDatabaseHashtable on this thread ? Yes, with the assumption that it shall only be touched in PBackgroundThread similar to what has been done in QuotaClient::AbortOperations(), QuotaClient::AbortOperationsForProcess, etc, I dispatch this runnable to BackgroundThread.
Comment 7•8 years ago
|
||
cool
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8761044 [details] [diff] [review] (v1) Patch: Support the 'close' Event on Databases. 1. Add QuotaClient::InvalidateDatabaseHelper to invalidate the databases according to the information provided from QuotaManager. 2. Report an error earlier for the IDBRequests received after database is invalidated to prevent further assertion failure in ConnectionPool::Dispatch(). 3. Trigger IDBDatabase.onclose() if invalidated from user agent.
Attachment #8761044 -
Flags: review?(khuey)
Comment 9•8 years ago
|
||
Comment on attachment 8761044 [details] [diff] [review] (v1) Patch: Support the 'close' Event on Databases. Review of attachment 8761044 [details] [diff] [review]: ----------------------------------------------------------------- We already have to enumerate the hashtable and go through all array elements and this patch adds a callback function call for each element. I'm not sure how it gets compiled in the end, but I guess it won't be inlined. ::: dom/indexedDB/ActorsParent.cpp @@ +17318,5 @@ > + return database->Origin() == origin && > + database->Type() == aPersistenceType; > + }); > + > + mBackgroundThread->Dispatch(runnable, NS_DISPATCH_NORMAL); Why do we actually need this ? It will invalidate all database objects right ? But those database objects were already invalidated in AbortOperations() prior deleting origin. OnOriginClearCompleted() is just used to cleanup some in-memory objects when origin clear is completely finished.
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8761044 [details] [diff] [review] (v1) Patch: Support the 'close' Event on Databases. Review of attachment 8761044 [details] [diff] [review]: ----------------------------------------------------------------- >Jan Varga [:janv] <jvarga@mozilla.com> >下午2:07:00 >----------------------------------------------------------------- >We already have to enumerate the hashtable and go through all array elements and this patch adds a callback function call for each element. I'm not sure how it gets compiled in the end, but I guess it won't be inlined. I am pretty sure that it's compiled because I've set the breakpoint on it and it stopped properly while running the test case with gdb. :) ::: dom/indexedDB/ActorsParent.cpp @@ +17318,5 @@ > + return database->Origin() == origin && > + database->Type() == aPersistenceType; > + }); > + > + mBackgroundThread->Dispatch(runnable, NS_DISPATCH_NORMAL); By my understanding, 1. nsIQuotaManagerService::clearStoragesForPrincipal() is the way how user agent clear the storage of a selected web site. (ForgetAboutSite.jsm) 2. OnOriginClearCompleted() is the callback of each QuotaClient for the notification of the completion of nsIQuotaManagerService::clearStoragesForPrincipal() instead of AbortOperations(). Hence, the implementation here is required to invalidate the IDB connection if its database was cleared by the user agent.
Comment 11•8 years ago
|
||
Ok, but we call Invalidate() on the same database objects in AbortOperations() and that happens before OnOriginClearCompleted(). Invalidate() checks for mInvalidated flag, so Invalidate() calls in OnOriginClearCompleted() should do nothing. I didn't apply your patch, but I think your patch currently doesn't fires the close event correctly, it's fired too early and the reason why next indexedDB.open() for the same origin and database works correctly is that it's protected by directory locks, so the next open() is delayed until clear origin is completed. The question is if we really need to fire the close event after origin clear finished.
Comment 12•8 years ago
|
||
If I read the spec correctly, the event can/should fire after Database::ConnectionClosedCallback() is called. In other words when all "aborted" transactions are completed.
Comment 13•8 years ago
|
||
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10) > I am pretty sure that it's compiled because I've set the breakpoint on it > and it stopped properly while running the test case with gdb. :) > I meant how it's compiled, standard function call versus substituting the call with function body.
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Jan Varga [:janv] from comment #11) > Ok, but we call Invalidate() on the same database objects in > AbortOperations() and that happens before OnOriginClearCompleted(). > Invalidate() checks for mInvalidated flag, so Invalidate() calls in > OnOriginClearCompleted() should do nothing. > Thanks for pointing out this! Just done more testing and found that the Database::Invalidate() is happened firstly at AbortOperations() instead of OnOriginClearCompleted(). I'll review the flow again to have the correct implementation of this close event! :)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8761044 [details] [diff] [review] (v1) Patch: Support the 'close' Event on Databases. cancel the review due to comment 11.
Attachment #8761044 -
Flags: review?(khuey)
Assignee | ||
Comment 16•8 years ago
|
||
Outline of this patch: 1. Define a new ipdl message called |CloseAfterInvalidationComplete| to trigger the close event after all transactions are complete only if the database is invalidated by the user agent. 2. Make sure the following event sequence is consistent during invalidation according to the steps in |5.2. Closing a database| by the following 2 solutions: IDBRequest.onerror -> IDBTransaction.onerror -> IDBTransaction.onabort -> IDBDatabase.onclose 2.1. In parent process, do not force to abort the transactions after invalidation but wait for all the transactions in its child process are complete. 2.2. In child process, make sure that each IDBTransaction will notify its completion to the parent after all its pending IDBRequests are finished. 3. Add test_database_onclose.js to test the close event especially when read/write operation is ongoing. 4. Add test_database_close_without_onclose.js as a XPCShell test because setTimeout() is not preferred in Mochitest to ensure that the IDBDatabase.onclose event won't be sent after closed normally. Result in treeherder for reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b5fd0331307
Attachment #8761044 -
Attachment is obsolete: true
Attachment #8765834 -
Flags: review?(khuey)
Comment on attachment 8765834 [details] [diff] [review] (v2) Patch: Support the 'close' Event on Databases. Review of attachment 8765834 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.cpp @@ -860,2 @@ > case IDBTransaction::READ_ONLY: > - break; This is important, why are you removing it? ::: dom/indexedDB/IDBTransaction.cpp @@ +375,5 @@ > MOZ_ASSERT(mPendingRequestCount); > > --mPendingRequestCount; > > + if (!mPendingRequestCount) { And why this change? @@ -647,5 @@ > -#endif > - // Increment the serial number counter here to account for the aborted > - // transaction and keep the parent in sync. > - IDBRequest::NextSerialNumber(); > - } And these?
Attachment #8765834 -
Flags: review?(khuey)
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17) > Comment on attachment 8765834 [details] [diff] [review] > (v2) Patch: Support the 'close' Event on Databases. > > Review of attachment 8765834 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/IDBDatabase.cpp > @@ -860,2 @@ > > case IDBTransaction::READ_ONLY: > > - break; > > This is important, why are you removing it? > > ::: dom/indexedDB/IDBTransaction.cpp > @@ +375,5 @@ > > MOZ_ASSERT(mPendingRequestCount); > > > > --mPendingRequestCount; > > > > + if (!mPendingRequestCount) { > > And why this change? > > @@ -647,5 @@ > > -#endif > > - // Increment the serial number counter here to account for the aborted > > - // transaction and keep the parent in sync. > > - IDBRequest::NextSerialNumber(); > > - } > > And these? In current implementation, we just ignore the read request without following the flow of |5.5. Steps for aborting a transaction| after invalidation. IMO, these changes are important to ensure that our implementation follows the flow of |5.2. Closing a database| in [1]. Without this change, after invalidation, 1. the pending request of reading will never be notified with onerror/onabort. 2. the read transaction won't be unregistered in ActorsParent properly. 2. the onclose event will never been sent to the IDBDatabase instance in which there is a read transaction ongoing because we still wait for the transaction complete in ActorsParent. (This is covered in test#3 of test_database_onclose.js.) The change in ActorsChild/ActorsParent is to ensure that the request and transaction will be aborted properly in sequence and the onclose will be sent after all the request/transaction are aborted properly. [1] http://w3c.github.io/IndexedDB/#closing-connection
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8765834 [details] [diff] [review] (v2) Patch: Support the 'close' Event on Databases. Review of attachment 8765834 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.cpp @@ -860,2 @@ > case IDBTransaction::READ_ONLY: > - break; All transactions has to run into the |Steps for aborting a transaction| after closing database. ::: dom/indexedDB/IDBTransaction.cpp @@ +375,5 @@ > MOZ_ASSERT(mPendingRequestCount); > > --mPendingRequestCount; > > + if (!mPendingRequestCount) { We have to notify this to ActorParent via SendAbort() to unregister this transaction in parent process. @@ -647,5 @@ > -#endif > - // Increment the serial number counter here to account for the aborted > - // transaction and keep the parent in sync. > - IDBRequest::NextSerialNumber(); > - } IDBRequest::NextSerialNumber() will be triggered in IDBTransaction::SendAbort() later in IDBTransaction::OnRequestFinished().
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8765834 [details] [diff] [review] (v2) Patch: Support the 'close' Event on Databases. Set r? again after clarified in comment 18 and comment 19.
Attachment #8765834 -
Flags: review?(khuey)
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8765834 [details] [diff] [review] (v2) Patch: Support the 'close' Event on Databases. Review of attachment 8765834 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsParent.cpp @@ -16350,5 @@ > } > > - if (mTransaction->IsInvalidated()) { > - return true; > - } To be consistent, the same check in Cursor::Start() shall be removed as well. I'll address this in next update.
Comment on attachment 8765834 [details] [diff] [review] (v2) Patch: Support the 'close' Event on Databases. Review of attachment 8765834 [details] [diff] [review]: ----------------------------------------------------------------- r- for the break thing, otherwise this looks pretty good. ::: dom/indexedDB/ActorsChild.cpp @@ +1878,5 @@ > + > + MaybeCollectGarbageOnIPCMessage(); > + > + if (mDatabase) { > + // Not bubble and not cancelable. The comment isn't necessary. ::: dom/indexedDB/ActorsChild.h @@ +419,5 @@ > virtual bool > RecvInvalidate() override; > > + > + virtual bool nit: extra \n above. ::: dom/indexedDB/IDBDatabase.cpp @@ -860,2 @@ > case IDBTransaction::READ_ONLY: > - break; This is just controlling what we report to the developer console. Note that we're breaking out of the switch, not the for loop above. We don't want to warn the web page author when a readonly transaction is aborted on window close/navigation because no data loss can result. See bug 1147942.
Attachment #8765834 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22) > ::: dom/indexedDB/IDBDatabase.cpp > @@ -860,2 @@ > > case IDBTransaction::READ_ONLY: > > - break; > > This is just controlling what we report to the developer console. Note that > we're breaking out of the switch, not the for loop above. We don't want to I must be blind when removing this... > warn the web page author when a readonly transaction is aborted on window > close/navigation because no data loss can result. See bug 1147942. Thanks for more explanation!
Assignee | ||
Comment 24•8 years ago
|
||
1. Remove the invalidation check in Cursor::Start() to have the consistent error handling as the one in Cursor::RecvContinue(). 2. Roll back the change in IDBDatabase::AbortTransactions() to suppress the warning of the abortion for readonly-mode transactions. 3. Increase the number of records to be read and mark as TODO instead of failure because there is no guarantee that the database will be cleared before all the records are iterated even we keep increasing the number of records to be read. (Hit one failure in today's treeherder result...)
Attachment #8765834 -
Attachment is obsolete: true
Attachment #8768363 -
Flags: review?(khuey)
Assignee | ||
Updated•8 years ago
|
Attachment #8768363 -
Attachment description: 1151017_v3.patch → (v3) Patch: Support the 'close' Event on Databases.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #23) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22) > > ::: dom/indexedDB/IDBDatabase.cpp > > @@ -860,2 @@ > > > case IDBTransaction::READ_ONLY: > > > - break; > > > > This is just controlling what we report to the developer console. Note that > > we're breaking out of the switch, not the for loop above. We don't want to > I must be blind when removing this... Happens to the best of us.
Attachment #8768363 -
Flags: review?(khuey) → review+
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 26•8 years ago
|
||
treeherder result looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49f31b12bb2
Keywords: checkin-needed
Comment 27•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/035505ea3149 Support the 'close' Event on Databases. r=khuey
Keywords: checkin-needed
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/035505ea3149
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 29•8 years ago
|
||
I’ve made the following changes to the documentation. I would appreciate it if someone could please review them. I’ve needinfo’d :bevistseng for his input in particular, but anyone else’s is welcome. New pages: https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase/onclose Pages updated: https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase (added onclose to the list of event handlers) https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB#Warning_about_browser_shutdown (updated text about what happens if the browser shuts down while a database is open — please let me know if this isn’t clear) Also updated: A couple of caveats: 1. Looking up historical information about Google releases is next to impossible; after a while, I gave up trying to find the exact version this was implemented in there. I know it was in July 2013, so I estimated Google 31. It might be 32. Might be 30. But 31 seems about right, so I went with it. 2. The specification links are not working yet. We use a macro to generate this and it hasn’t been updated yet to know that the spec has a version 2.0. I’m talking to the other writers to ensure that this is updated the right way. Those caveats aside, this is done, so I’m marking it as doc complete.
Flags: needinfo?(btseng)
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 30•8 years ago
|
||
Looks good to me. Thanks for having these documented.
Flags: needinfo?(btseng)
You need to log in
before you can comment on or make changes to this bug.
Description
•