Bug 203431 - REGRESSION (r250754): web page using IDBIndex doesn't load occasionally
Summary: REGRESSION (r250754): web page using IDBIndex doesn't load occasionally
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-25 13:00 PDT by Sihui Liu
Modified: 2019-11-05 10:26 PST (History)
12 users (show)

See Also:


Attachments
Patch (37.58 KB, patch)
2019-10-31 15:37 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (43.66 KB, patch)
2019-10-31 15:58 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (43.73 KB, patch)
2019-11-01 17:15 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (54.33 KB, patch)
2019-11-04 20:21 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (53.87 KB, patch)
2019-11-05 10:01 PST, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-10-25 13:00:15 PDT
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
Comment 1 Sihui Liu 2019-10-25 13:26:08 PDT
Adding shouldPreventEnteringBackForwardCache_DEPRECATED() to return true in IDBIndex fixes the issue.
Comment 2 Chris Dumez 2019-10-25 13:28:42 PDT
(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.
Comment 3 Sihui Liu 2019-10-25 18:30:11 PDT
(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().
Comment 4 Sihui Liu 2019-10-25 18:33:46 PDT
(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
Comment 5 Chris Dumez 2019-10-25 18:45:45 PDT
(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.
Comment 6 Chris Dumez 2019-10-28 12:35:33 PDT
Sihui, are you looking into fixing this?
Comment 7 Sihui Liu 2019-10-31 15:37:58 PDT
Created attachment 382510 [details]
Patch
Comment 8 Sihui Liu 2019-10-31 15:58:40 PDT
Created attachment 382514 [details]
Patch
Comment 9 Sihui Liu 2019-11-01 17:15:39 PDT
Created attachment 382659 [details]
Patch
Comment 10 Chris Dumez 2019-11-02 11:59:41 PDT
Comment on attachment 382659 [details]
Patch

r- due to EWS failures (crashes look related).
Comment 11 Sihui Liu 2019-11-04 10:46:42 PST
(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.
Comment 12 Brady Eidson 2019-11-04 12:55:14 PST
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.
Comment 13 Chris Dumez 2019-11-04 12:58:27 PST
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.
Comment 14 Chris Dumez 2019-11-04 13:07:02 PST
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.
Comment 15 Brady Eidson 2019-11-04 13:21:09 PST
(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.
Comment 16 Sihui Liu 2019-11-04 20:21:05 PST
Created attachment 382804 [details]
Patch
Comment 17 Alex Christensen 2019-11-04 20:23:56 PST
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.
Comment 18 Sihui Liu 2019-11-05 10:01:38 PST
Created attachment 382831 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2019-11-05 10:25:52 PST
Comment on attachment 382831 [details]
Patch for landing

Clearing flags on attachment: 382831

Committed r252060: <https://trac.webkit.org/changeset/252060>
Comment 20 WebKit Commit Bot 2019-11-05 10:25:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-11-05 10:26:29 PST
<rdar://problem/56909138>