Bug 215271

Summary: [ macOS iOS wk 2 Debug ] storage/indexeddb/modern/new-database-after-user-delete.html is a flaky failure
Product: WebKit Reporter: Hector Lopez <hector_i_lopez>
Component: New BugsAssignee: Cameron McCormack (:heycam) <heycam>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, ews-watchlist, heycam, jsbell, pvollan, sihui_liu, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 221124    
Attachments:
Description Flags
Patch
none
Patch none

Description Hector Lopez 2020-08-07 08:46:46 PDT 
storage/indexeddb/modern/new-database-after-user-delete.html

This test is a flaky failure according to history. First occurrence of failure on iOS is at r259341. First occurrence of failure for macOS is at r251207.

History:
https://results.webkit.org/?suite=layout-tests&test=storage%2Findexeddb%2Fmodern%2Fnew-database-after-user-delete.html&platform=ios&platform=mac&limit=50000&style=debug

Diff:
--- /Volumes/Data/slave/ipados-simulator-13-debug-tests-wk2/build/layout-test-results/storage/indexeddb/modern/new-database-after-user-delete-expected.txt
+++ /Volumes/Data/slave/ipados-simulator-13-debug-tests-wk2/build/layout-test-results/storage/indexeddb/modern/new-database-after-user-delete-actual.txt
@@ -13,6 +13,7 @@
 Version change transaction completed. Version is now 1. About to request clearAllDatabases
 Requested clearAllDatabases
 Database connection error'ed out. Opening a new connection.
+Database connection error'ed out. Opening a new connection.
 Reopened upgrade needed: Old version - 0 New version - 1
 [PASS] The database has no object stores, meaning it is new
 PASS successfullyParsed is true
Comment 1 Radar WebKit Bug Importer 2020-08-07 08:55:48 PDT 
<rdar://problem/66683545>
Comment 2 Hector Lopez 2020-08-07 08:56:27 PDT 
Test expectations while investigated:

https://trac.webkit.org/changeset/265374/webkit
Comment 3 Cameron McCormack (:heycam) 2022-03-15 23:27:25 PDT 
*** Bug 204489 has been marked as a duplicate of this bug. ***
Comment 4 Cameron McCormack (:heycam) 2022-03-15 23:45:27 PDT 
*** Bug 204489 has been marked as a duplicate of this bug. ***
Comment 5 Cameron McCormack (:heycam) 2022-03-15 23:47:02 PDT 
Created attachment 454805 [details]
Patch
Comment 6 Sihui Liu 2022-03-16 07:56:12 PDT 
Comment on attachment 454805 [details]
Patch

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

r=me

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1460
> +    ASSERT(!m_versionChangeDatabaseConnection || openDatabaseConnections.contains(m_versionChangeDatabaseConnection.get()));
> +    m_versionChangeDatabaseConnection = nullptr;

hmm, the mac-AS-debug-wk2 seems related.

oh I see, there is still a case where we don't add m_versionChangeDatabaseConnection to m_openDatabaseConnections synchronously:
in UniqueIDBDatabase::performCurrentOpenOperationAfterSpaceCheck and when it falls to maybeNotifyConnectionsOfVersionChange() -- we would add it to the set after events are dispatched to existing open connections.

Let's change this to check if openDatabaseConnections has m_versionChangeDatabaseConnection.get(), and close it if it's not in the set and has not been closed.
Comment 7 Sihui Liu 2022-03-16 07:57:04 PDT 
(In reply to Sihui Liu from comment #6)
> Comment on attachment 454805 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454805&action=review
> 
> r=me

(Sorry was about to to say r=me after the change)

> 
> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1460
> > +    ASSERT(!m_versionChangeDatabaseConnection || openDatabaseConnections.contains(m_versionChangeDatabaseConnection.get()));
> > +    m_versionChangeDatabaseConnection = nullptr;
> 
> hmm, the mac-AS-debug-wk2 seems related.
> 
> oh I see, there is still a case where we don't add
> m_versionChangeDatabaseConnection to m_openDatabaseConnections synchronously:
> in UniqueIDBDatabase::performCurrentOpenOperationAfterSpaceCheck and when it
> falls to maybeNotifyConnectionsOfVersionChange() -- we would add it to the
> set after events are dispatched to existing open connections.
> 
> Let's change this to check if openDatabaseConnections has
> m_versionChangeDatabaseConnection.get(), and close it if it's not in the set
> and has not been closed.
Comment 8 Cameron McCormack (:heycam) 2022-03-16 15:26:49 PDT 
Created attachment 454902 [details]
Patch
Comment 9 Sihui Liu 2022-03-16 15:30:41 PDT 
Comment on attachment 454902 [details]
Patch

r=me, let's wait for bots to be green before landing
Comment 10 EWS 2022-03-17 13:20:39 PDT 
Committed r291435 (248559@main): <https://commits.webkit.org/248559@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454902 [details].