IndexedDB operations in a Page fail after a StorageProcess crash

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dawei Fenton (:realdawei)]

There are a bunch of tests in a the imported/w3c/web-platform-tests/IndexedDB/* when when one fails the majority will fail in the same way (generally over 300 tests will fail).

Possible issue could be state being retained in the TestRunner.

Sample Run:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/5203/steps/layout-test/logs/stdio

[Jonathan Bedard]

Created attachment 343766 [details]
Test order to reproduce

[Dawei Fenton (:realdawei)]

(In reply to Jonathan Bedard from comment #1)
> Created attachment 343766 [details]
> Test order to reproduce

after further testing it turns out if these tests are performed in the same run (in any order) the test will fail.  When any one of tests is removed it will pass.

http/wpt/service-workers/persistent-importScripts.html
http/wpt/cache-storage/cache-put-keys.https.any.worker.html
imported/w3c/web-platform-tests/IndexedDB/close-in-upgradeneeded.html

sample output:
Regressions: Unexpected text-only failures (1)
  imported/w3c/web-platform-tests/IndexedDB/close-in-upgradeneeded.html [ Failure ]

[Jonathan Bedard]

Given that we haven't seen this until recently, I would suspect that <https://trac.webkit.org/changeset/232516/webkit> is the root cause.

[Radar WebKit Bug Importer]

<rdar://problem/41626526>

[Brady Eidson]

Pages and contexts that use IDB (Documents, Workers, etc) do not recover well from a StorageProcess crash.

They don't crash, but they're unaware that the StorageProcess is gone and therefore their messages to do IDB stuff are lost in the ether and never answered.

I have a fix to make Pages recover from this and send contexts into a mode where requests after the storage process crash will at least error out immediately.

[Brady Eidson]

Created attachment 344354 [details]
Patch

[Brady Eidson]

Haven't even finished building this patch locally, but it should be final or near final. Trying EWS.

[Brady Eidson]

(whoops forgot test expectations)

[Brady Eidson]

Created attachment 344359 [details]
Patch

[Alex Christensen]

Comment on attachment 344359 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:67
> +void IDBConnectionToServer::callResultFunctionLater(ResultFunction function, const IDBResourceIdentifier& requestIdentifier)

I think this name should indicate that it will be called with an error.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:364
> +        callOnMainThread([this, protectedThis = makeRef(*this), transactionIdentifier] {

Why can't these use callResultFunctionLater?

> Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:502
> +    if (m_serverConnectionIsValid)
> +        m_delegate->abortOpenAndUpgradeNeeded(databaseConnectionIdentifier, transactionIdentifier);

Couldn't the delegate check if its connection is valid instead of all these checks here?

> LayoutTests/storage/indexeddb/modern/opendatabase-after-storage-crash.html:49
> +		// Padding it makes this reliable.

how reliable?

[Brady Eidson]

(In reply to Alex Christensen from comment #10)
> Comment on attachment 344359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344359&action=review
> 
> > Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:67
> > +void IDBConnectionToServer::callResultFunctionLater(ResultFunction function, const IDBResourceIdentifier& requestIdentifier)
> 
> I think this name should indicate that it will be called with an error.

Sure.

> 
> > Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:364
> > +        callOnMainThread([this, protectedThis = makeRef(*this), transactionIdentifier] {
> 
> Why can't these use callResultFunctionLater?

Because they have custom call signatures, not IDBResults.

I played with full lambdas, full templetization, and mixes - This was the cleanest/most compact form.

> 
> > Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:502
> > +    if (m_serverConnectionIsValid)
> > +        m_delegate->abortOpenAndUpgradeNeeded(databaseConnectionIdentifier, transactionIdentifier);
> 
> Couldn't the delegate check if its connection is valid instead of all these
> checks here?

That'd require a patch over twice as large, as we'd have to cover 2 delegates (WK1 and WK2)

> 
> > LayoutTests/storage/indexeddb/modern/opendatabase-after-storage-crash.html:49
> > +		// Padding it makes this reliable.
> 
> how reliable?

100% reliable™

[Brady Eidson]

Created attachment 344362 [details]
Patch

[Alex Christensen]

Comment on attachment 344362 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:67
> +void IDBConnectionToServer::callResultFunctionWithErrorLater(ResultFunction function, const IDBResourceIdentifier& requestIdentifier)

It could be made more compact by passing the const IDBRequestData&

[Brady Eidson]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 344362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344362&action=review
> 
> > Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:67
> > +void IDBConnectionToServer::callResultFunctionWithErrorLater(ResultFunction function, const IDBResourceIdentifier& requestIdentifier)
> 
> It could be made more compact by passing the const IDBRequestData&

Fewer characters in the signature, more in the function body. /me shrugs

[EWS Watchlist]

Comment on attachment 344362 [details]
Patch

Attachment 344362 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8449520

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
http/tests/security/local-video-source-from-remote.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344375 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Brady Eidson]

The commit queue is in quite a state right now, that's for sure.

[Brady Eidson]

The EWS bots were clearly just in a bad state... let's try this again.

[Brady Eidson]

Created attachment 344397 [details]
Patch for EWS/Landing

[EWS Watchlist]

Comment on attachment 344397 [details]
Patch for EWS/Landing

Attachment 344397 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8452509

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 344401 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Brady Eidson]

Nope this patch did not cause that failurw

[WebKit Commit Bot]

Comment on attachment 344397 [details]
Patch for EWS/Landing

Clearing flags on attachment: 344397

Committed r233562: <https://trac.webkit.org/changeset/233562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the test storage/indexeddb/modern/opendatabase-after-storage-crash.html
is failing both with timeouts and text failures 

test history: 
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=storage%2Findexeddb%2Fmodern%2Fopendatabase-after-storage-crash.html

[Truitt Savell]

UPDATE:
It looks like the test storage/indexeddb/modern/opendatabase-after-storage-crash.html
is still a flakey failure, though the timeouts seem to be largely resolved. 

test history: 
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=storage%2Findexeddb%2Fmodern%2Fopendatabase-after-storage-crash.html

[Alexey Proskuryakov]

The test is still failing about 50% of the time, tracked in bug 187648.