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
Adding shouldPreventEnteringBackForwardCache_DEPRECATED() to return true in IDBIndex fixes the issue.
(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.
(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().
(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
(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.
Sihui, are you looking into fixing this?
Created attachment 382510 [details] Patch
Created attachment 382514 [details] Patch
Created attachment 382659 [details] Patch
Comment on attachment 382659 [details] Patch r- due to EWS failures (crashes look related).
(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 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 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 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.
(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.
Created attachment 382804 [details] Patch
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.
Created attachment 382831 [details] Patch for landing
Comment on attachment 382831 [details] Patch for landing Clearing flags on attachment: 382831 Committed r252060: <https://trac.webkit.org/changeset/252060>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56909138>