Closed Bug 1151017 Opened 9 years ago Closed 8 years ago

Support the "close" event on databases

Categories

(Core :: Storage: IndexedDB, defect)

Product:
Core
Component:
Storage: IndexedDB
Platform:
x86
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: bent.mozilla, Assigned: bevis, Mentored)

References

(
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file, 2 obsolete files)

(v3) Patch: Support the 'close' Event on Databases.
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.
I'd like to take this bug to be more familiar with IndexedDB-related implementation.
Assignee: nobody → btseng
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.
Status: NEW → ASSIGNED
Whiteboard: [tw-dom] → [tw-dom] btpp-active
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 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 ?
(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.
cool
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 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.
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.
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.
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.
(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.
(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! :)
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)
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)
(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
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().
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)
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-
(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!
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)
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.
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
https://hg.mozilla.org/mozilla-central/rev/035505ea3149
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Blocks: 1271497
Looks good to me.
Thanks for having these documented.
Flags: needinfo?(btseng)
Depends on: 1300454
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: