Bug 178494 – http/tests/workers/service/basic-register.html is a flaky failure.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Matt Lewis]

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!

[Radar WebKit Bug Importer]

<rdar://problem/35065315>

[Chris Dumez]

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.

[youenn fablet]

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.

[Chris Dumez]

Found the problem. Patch is coming shortly.

[Chris Dumez]

Created attachment 324203 [details]
Patch

[youenn fablet]

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?

[Chris Dumez]

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.

[Chris Dumez]

Comment on attachment 324203 [details]
Patch

Clearing flags on attachment: 324203

Committed r223690: <https://trac.webkit.org/changeset/223690>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.