Bug 203431 – REGRESSION (r250754): web page using IDBIndex doesn't load occasionally

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

Reproducible steps:
1. Open local test page in minibrowser: OpenSource/PerformanceTests/IndexedDB/basic/index-count-key.html
2. Nagivate to OpenSource/PerformanceTests/IndexedDB/basic/index-count.html (address bar)

Result: second page does not load

[Sihui Liu]

Adding shouldPreventEnteringBackForwardCache_DEPRECATED() to return true in IDBIndex fixes the issue.

[Chris Dumez]

(In reply to Sihui Liu from comment #1)
> Adding shouldPreventEnteringBackForwardCache_DEPRECATED() to return true in
> IDBIndex fixes the issue.

This would not the right fix though, just to be clear.

[Sihui Liu]

(In reply to Chris Dumez from comment #2)
> (In reply to Sihui Liu from comment #1)
> > Adding shouldPreventEnteringBackForwardCache_DEPRECATED() to return true in
> > IDBIndex fixes the issue.
> 
> This would not the right fix though, just to be clear.

The cause is the IDBDatabase object is not destroyed and database connection stays open when user navigates to another page. When the second page tries to delete the database, it's blocked on the open connection.

We have to close database connections on navigation, and this would change the state of the IDB DOM objects (open connection -> closed, ongoing transaction -> aborted, etc). If the state change is acceptable for page cache, we probably stop()/close() the IDBDatabase in suspend().

[Sihui Liu]

(In reply to Sihui Liu from comment #3)
> (In reply to Chris Dumez from comment #2)
> > (In reply to Sihui Liu from comment #1)
> > > Adding shouldPreventEnteringBackForwardCache_DEPRECATED() to return true in
> > > IDBIndex fixes the issue.
> > 
> > This would not the right fix though, just to be clear.
> 
> The cause is the IDBDatabase object is not destroyed and database connection
> stays open when user navigates to another page. When the second page tries
> to delete the database, it's blocked on the open connection.
> 
> We have to close database connections on navigation, and this would change
> the state of the IDB DOM objects (open connection -> closed, ongoing
> transaction -> aborted, etc). If the state change is acceptable for page
> cache, we probably stop()/close() the IDBDatabase in suspend().

Or maybe to invalidate the IDB objects when there are IDB changes from the new page, and discard page cache when there are invalidated IDB objects

[Chris Dumez]

(In reply to Sihui Liu from comment #4)
> (In reply to Sihui Liu from comment #3)
> > (In reply to Chris Dumez from comment #2)
> > > (In reply to Sihui Liu from comment #1)
> > > > Adding shouldPreventEnteringBackForwardCache_DEPRECATED() to return true in
> > > > IDBIndex fixes the issue.
> > > 
> > > This would not the right fix though, just to be clear.
> > 
> > The cause is the IDBDatabase object is not destroyed and database connection
> > stays open when user navigates to another page. When the second page tries
> > to delete the database, it's blocked on the open connection.
> > 
> > We have to close database connections on navigation, and this would change
> > the state of the IDB DOM objects (open connection -> closed, ongoing
> > transaction -> aborted, etc). If the state change is acceptable for page
> > cache, we probably stop()/close() the IDBDatabase in suspend().
> 
> Or maybe to invalidate the IDB objects when there are IDB changes from the
> new page, and discard page cache when there are invalidated IDB objects

We need to be careful about Web compatibility.

I looked at the spec and found:
"""
A connection may be closed by a user agent in exceptional circumstances, for example due to loss of access to the file system, a permission change, or clearing of the origin’s storage. If this occurs the user agent must run close a database connection with the connection and with the forced flag set to true.
"""

It looks like "close a database connection" algorithm fires a "close" event so I guess the page would at least get notified on resume that the connection was closed and could re-open it itself.

Ideally though, the Web page would not have to do anything and things would recover nicely by themselves upon resuming. That said, I have no idea if it is feasible here.

[Chris Dumez]

Sihui, are you looking into fixing this?

[Sihui Liu]

Created attachment 382510 [details]
Patch

[Sihui Liu]

Created attachment 382514 [details]
Patch

[Sihui Liu]

Created attachment 382659 [details]
Patch

[Chris Dumez]

Comment on attachment 382659 [details]
Patch

r- due to EWS failures (crashes look related).

[Sihui Liu]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 382659 [details]
> Patch
> 
> r- due to EWS failures (crashes look related).

That crash is not caused by this patch: https://bugs.webkit.org/show_bug.cgi?id=201744. It's been around for a while and I've not reproduced successfully on a local build yet.

[Brady Eidson]

Comment on attachment 382659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382659&action=review

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:470
> +    if (scriptExecutionContext()->isDocument()) {

If the global object that has the open database handle is a web worker (i.e. not a Document) then it will not do the right thing here.

IDB is frequently used by workers, so we definitely have to handle that case.

I wonder if Workers know if their owning document is in the page cache?
If not, I wonder how hard it would be to teach them?

Regardless, we need to solve that.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h:88
> +    void didFireVersionChangeEvent(uint64_t databaseConnectionIdentifier, const IDBResourceIdentifier& requestIdentifier, bool connectionClosedOnBehalfOfServer);

Hard to read with trues and falses scattered about.
Let's make the bool be an enum.

[Chris Dumez]

Comment on attachment 382659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382659&action=review

>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:470
>> +    if (scriptExecutionContext()->isDocument()) {
> 
> If the global object that has the open database handle is a web worker (i.e. not a Document) then it will not do the right thing here.
> 
> IDB is frequently used by workers, so we definitely have to handle that case.
> 
> I wonder if Workers know if their owning document is in the page cache?
> If not, I wonder how hard it would be to teach them?
> 
> Regardless, we need to solve that.

Workers do not know that their document is in the page cache. When their document is in the page cache, the worker thread is paused and therefore no code can run.

[Chris Dumez]

Comment on attachment 382659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382659&action=review

>>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:470
>>> +    if (scriptExecutionContext()->isDocument()) {
>> 
>> If the global object that has the open database handle is a web worker (i.e. not a Document) then it will not do the right thing here.
>> 
>> IDB is frequently used by workers, so we definitely have to handle that case.
>> 
>> I wonder if Workers know if their owning document is in the page cache?
>> If not, I wonder how hard it would be to teach them?
>> 
>> Regardless, we need to solve that.
> 
> Workers do not know that their document is in the page cache. When their document is in the page cache, the worker thread is paused and therefore no code can run.

This means that if someones posts a task on the worker thread to run IDBDatabase::fireVersionChangeEvent(), that task won't be processed until *after* the document has been restored from the backforward cache.

[Brady Eidson]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 382659 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382659&action=review
> 
> >>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:470
> >>> +    if (scriptExecutionContext()->isDocument()) {
> >> 
> >> If the global object that has the open database handle is a web worker (i.e. not a Document) then it will not do the right thing here.
> >> 
> >> IDB is frequently used by workers, so we definitely have to handle that case.
> >> 
> >> I wonder if Workers know if their owning document is in the page cache?
> >> If not, I wonder how hard it would be to teach them?
> >> 
> >> Regardless, we need to solve that.
> > 
> > Workers do not know that their document is in the page cache. When their document is in the page cache, the worker thread is paused and therefore no code can run.
> 
> This means that if someones posts a task on the worker thread to run
> IDBDatabase::fireVersionChangeEvent(), that task won't be processed until
> *after* the document has been restored from the backforward cache.

Right - And that means the new page's IDB use would be blocked just like it is today.

Sihui and I figured a good fix for this case IRL. I think she's implementing it now.

[Sihui Liu]

Created attachment 382804 [details]
Patch

[Alex Christensen]

Comment on attachment 382804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382804&action=review

> Source/WebCore/Modules/indexeddb/IndexedDB.h:127
> +template<> struct EnumTraits<WebCore::IndexedDB::ConnectionClosedOnBehalfOfServer> {

If you use bool as the storage type for this enum class, then it will be smaller and this will be unnecessary.

[Sihui Liu]

Created attachment 382831 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 382831 [details]
Patch for landing

Clearing flags on attachment: 382831

Committed r252060: <https://trac.webkit.org/changeset/252060>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56909138>