Bug 178494 - http/tests/workers/service/basic-register.html is a flaky failure.
Summary: http/tests/workers/service/basic-register.html is a flaky failure.
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: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-18 17:58 PDT by Matt Lewis
Modified: 2017-10-19 09:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.79 KB, patch)
2017-10-18 19:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lewis 2017-10-18 17:58:35 PDT
http/tests/workers/service/basic-register.html is a flaky failure according to the dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fbasic-register.html

build:
https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r223608%20(5296)/results.html
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/5296

diff:

--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/http/tests/workers/service/basic-register-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/http/tests/workers/service/basic-register-actual.txt
@@ -1,7 +1,7 @@
 PASS: No service worker is initially registered for this origin
 PASS: No service worker is initially registered for this origin in private session
 Registered!
-PASS: A service worker is now registered for this origin
+FAIL: No service worker is registered for this origin
 PASS: No service worker is registered for this origin in private session
 Registered!
Comment 1 Radar WebKit Bug Importer 2017-10-18 17:58:51 PDT
<rdar://problem/35065315>
Comment 2 Chris Dumez 2017-10-18 18:43:26 PDT
I cannot reproduce yet. My bet is that there is a race and we sometimes check the store from the WebProcess before it has received the shared memory handle from the Storage Process.
Comment 3 youenn fablet 2017-10-18 18:46:32 PDT
I saw some issues with service workers tests when running all http/tests/workers tests with iterations=5 and -f.
Do not remember whether this was this particular test.
Comment 4 Chris Dumez 2017-10-18 19:24:03 PDT
Found the problem. Patch is coming shortly.
Comment 5 Chris Dumez 2017-10-18 19:54:26 PDT
Created attachment 324203 [details]
Patch
Comment 6 youenn fablet 2017-10-19 08:25:54 PDT
Comment on attachment 324203 [details]
Patch

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

> Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:47
> +    m_store.flushPendingChanges();

scheduleAddition will add to the pending list and will start the timer.
Then flushPendingChanges will stop the timer and process the operations.
Is there a way to simplify this?

> Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:50
> +void WebSWOriginStore::addAll(const Vector<SecurityOrigin>& origins)

addAll is not used AFAICT.
If we keep it and have a removeAll, is there still a need for the timer?
Comment 7 Chris Dumez 2017-10-19 09:10:09 PDT
Comment on attachment 324203 [details]
Patch

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

>> Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:47
>> +    m_store.flushPendingChanges();
> 
> scheduleAddition will add to the pending list and will start the timer.
> Then flushPendingChanges will stop the timer and process the operations.
> Is there a way to simplify this?

Yes, I will look into factoring this better in a follow-up. I need to consider the VisitedLinkHashStore which uses the same data structure.

>> Source/WebKit/StorageProcess/ServiceWorker/WebSWOriginStore.cpp:50
>> +void WebSWOriginStore::addAll(const Vector<SecurityOrigin>& origins)
> 
> addAll is not used AFAICT.
> If we keep it and have a removeAll, is there still a need for the timer?

The Timer is at the lower level object (SharedStringHashStore), which is used by the VisitedLinkHashStore. This is where the timer is coming from.
Comment 8 Chris Dumez 2017-10-19 09:10:51 PDT
Comment on attachment 324203 [details]
Patch

Clearing flags on attachment: 324203

Committed r223690: <https://trac.webkit.org/changeset/223690>
Comment 9 Chris Dumez 2017-10-19 09:10:53 PDT
All reviewed patches have been landed.  Closing bug.