| Summary: | [ WK2 ] http/tests/workers/service/client-*-page-cache.html LayoutTests are flaky | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||||
| Component: | Service Workers | Assignee: | Chris Dumez <cdumez> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | beidson, cdumez, commit-queue, jlewis3, tsavell, webkit-bug-importer, wenson_hsieh, youennf | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ryan Haddad
2018-03-16 13:03:58 PDT
These two tests currently became extremely flaky along with a third. http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html http/tests/workers/service/serviceworkerclients-matchAll.https.html Combined History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fserviceworkerclients-matchAll.https.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-added-to-clients-when-restored-from-page-cache.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-removed-from-clients-while-in-page-cache.html They all began failing around r233895. serviceworkerclients-matchAll.https.html Text Diff: --- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/serviceworkerclients-matchAll.https-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/serviceworkerclients-matchAll.https-actual.txt @@ -1,4 +1,4 @@ PASS Setup worker -PASS Test self.clients.matchAll +FAIL Test self.clients.matchAll assert_equals: expected "PASS" but got "FAIL: expected one matched client, got 2" These issues seem to be due to the test receiving one additional client than they expected. Issue seems to have began between r233891 -- 233897. r233891 has specific changes to Clients: https://trac.webkit.org/changeset/233891/webkit (In reply to Truitt Savell from comment #2) > Issue seems to have began between r233891 -- 233897. > > r233891 has specific changes to Clients: > https://trac.webkit.org/changeset/233891/webkit I really doubt this is the cause. This patch has no behavior change unless clients opt into the new SPI, which WebKitTestRunner does not. (In reply to Wenson Hsieh from comment #3) > (In reply to Truitt Savell from comment #2) > > Issue seems to have began between r233891 -- 233897. > > > > r233891 has specific changes to Clients: > > https://trac.webkit.org/changeset/233891/webkit > > I really doubt this is the cause. This patch has no behavior change unless > clients opt into the new SPI, which WebKitTestRunner does not. I am working on a bad crasher right now but I am happy to look into this bug right after that. I noticed that http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html always fail together. They may be getting stuck in a state together. Or because they getting more clients than they expect maybe something is providing too many clients earlier in the run. Ok, I have some spare cycles to look into this. > Issue seems to have began between r233891 -- 233897. Likely a regression from process-swap-on-navigation then, which was r233897. (In reply to Chris Dumez from comment #7) > > Issue seems to have began between r233891 -- 233897. > > Likely a regression from process-swap-on-navigation then, which was r233897. Unable to reproduce so far: Tools/Scripts/run-webkit-tests http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html http/tests/workers/service/client-removed-from-clients-while-in-page-cache.html --repeat-each=100 -f Also tried running the whole folder. Created attachment 345463 [details]
Patch
Comment on attachment 345463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345463&action=review > LayoutTests/http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache.html:17 > + worker.postMessage("getClientCount"); Shouldn't we do the usual pattern: if (++counter < 20) { log("FAIL...") finishSWTest(); } await waitFor(50); worker.postMessage("getClientCount"); > LayoutTests/http/tests/workers/service/serviceworkerclients-matchAll-worker.js:26 > + result = await matchAllPromise1; This loop does not seem useful as is. We might want to change matchAllPromise1 to a function which would return a new promise every time it is called. Again a waitFor(50) might help. Created attachment 345470 [details]
Patch
Comment on attachment 345470 [details] Patch Clearing flags on attachment: 345470 Committed r234061: <https://trac.webkit.org/changeset/234061> All reviewed patches have been landed. Closing bug. (In reply to WebKit Commit Bot from comment #12) > Comment on attachment 345470 [details] > Patch > > Clearing flags on attachment: 345470 > > Committed r234061: <https://trac.webkit.org/changeset/234061> It does not look like this change made a difference. Test is still flakey and behavior seems to be the same. failure message is also the same. reopening for visibility (In reply to Truitt Savell from comment #15) > (In reply to WebKit Commit Bot from comment #12) > > Comment on attachment 345470 [details] > > Patch > > > > Clearing flags on attachment: 345470 > > > > Committed r234061: <https://trac.webkit.org/changeset/234061> > > It does not look like this change made a difference. Test is still flakey > and behavior seems to be the same. failure message is also the same. Which test? What is the output? (In reply to Chris Dumez from comment #17) > (In reply to Truitt Savell from comment #15) > > (In reply to WebKit Commit Bot from comment #12) > > > Comment on attachment 345470 [details] > > > Patch > > > > > > Clearing flags on attachment: 345470 > > > > > > Committed r234061: <https://trac.webkit.org/changeset/234061> > > > > It does not look like this change made a difference. Test is still flakey > > and behavior seems to be the same. failure message is also the same. > > Which test? What is the output? The 2 page cache tests: --- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/client-removed-from-clients-while-in-page-cache-expected.txt +++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/workers/service/client-removed-from-clients-while-in-page-cache-actual.txt @@ -1,6 +1,4 @@ * Tests that a client is removed from the list of service worker clients while it is in the page cache -PASS: service worker has initially 2 clients -PASS: page is about to enter page cache -PASS: service worker has only 1 client after 1 entered page cache +FAIL: Wrong initial number of clients: 3 (In reply to Chris Dumez from comment #17) > (In reply to Truitt Savell from comment #15) > > (In reply to WebKit Commit Bot from comment #12) > > > Comment on attachment 345470 [details] > > > Patch > > > > > > Clearing flags on attachment: 345470 > > > > > > Committed r234061: <https://trac.webkit.org/changeset/234061> > > > > It does not look like this change made a difference. Test is still flakey > > and behavior seems to be the same. failure message is also the same. > > Which test? What is the output? Sorry I meant tests. All three tests: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fserviceworkerclients-matchAll.https.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-added-to-clients-when-restored-from-page-cache.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-removed-from-clients-while-in-page-cache.html are still having flakey failures. Test Outputs: https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r234067%20(5511)/http/tests/workers/service/serviceworkerclients-matchAll.https-pretty-diff.html https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r234064%20(10678)/http/tests/workers/service/client-added-to-clients-when-restored-from-page-cache-pretty-diff.html (In reply to Truitt Savell from comment #19) > (In reply to Chris Dumez from comment #17) > > (In reply to Truitt Savell from comment #15) > > > (In reply to WebKit Commit Bot from comment #12) > > > > Comment on attachment 345470 [details] > > > > Patch > > > > > > > > Clearing flags on attachment: 345470 > > > > > > > > Committed r234061: <https://trac.webkit.org/changeset/234061> > > > > > > It does not look like this change made a difference. Test is still flakey > > > and behavior seems to be the same. failure message is also the same. > > > > Which test? What is the output? > > Sorry I meant tests. All three tests: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fserviceworkerc > lients-matchAll.https.html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient- > added-to-clients-when-restored-from-page-cache. > html%20%20http%2Ftests%2Fworkers%2Fservice%2Fclient-removed-from-clients- > while-in-page-cache.html > > are still having flakey failures. > > Test Outputs: > https://build.webkit.org/results/ > Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r234067%20(5511)/http/ > tests/workers/service/serviceworkerclients-matchAll.https-pretty-diff.html > > https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > r234064%20(10678)/http/tests/workers/service/client-added-to-clients-when- > restored-from-page-cache-pretty-diff.html So maybe the clients are actually leaking in some cases? :/ Marked tests as flaky in https://trac.webkit.org/r234100 Committed r234107: <https://trac.webkit.org/changeset/234107> Reopening as this latest fix was only for http/tests/workers/service/serviceworkerclients-matchAll.https.html. It is works, I'll apply the same fix to the other 2 tests. (In reply to Chris Dumez from comment #23) > Reopening as this latest fix was only for > http/tests/workers/service/serviceworkerclients-matchAll.https.html. It is > works, I'll apply the same fix to the other 2 tests. Here is a failure on iOS Simulator after r234107: https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r234109%20(6374)/results.html Marked http/tests/workers/service/serviceworkerclients-matchAll.https.html as flaky again in https://trac.webkit.org/r234170. (In reply to Ryan Haddad from comment #25) > Marked http/tests/workers/service/serviceworkerclients-matchAll.https.html > as flaky again in https://trac.webkit.org/r234170. Ok thanks. I will look into this again when I am done with more urgent matters. Created attachment 348671 [details]
Patch
Comment on attachment 348671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348671&action=review > Source/WebCore/testing/Internals.cpp:2331 > +String Internals::serviceWorkerClientIdentifier(Document& document) const const Document& maybe? > LayoutTests/ChangeLog:10 > + identifiers to see if they are present or not. Were these tests testable in a regular browser before? Would it make sense to use internals API if available and rely on client count otherwise? Looking further for some tests, for controlled/uncontrolled clients, it makes sense to use identifiers. Comment on attachment 348671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348671&action=review >> Source/WebCore/testing/Internals.cpp:2331 >> +String Internals::serviceWorkerClientIdentifier(Document& document) const > > const Document& maybe? Canno Comment on attachment 348671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348671&action=review >>> Source/WebCore/testing/Internals.cpp:2331 >>> +String Internals::serviceWorkerClientIdentifier(Document& document) const >> >> const Document& maybe? > > Canno Oh, my previous implementation needed this non-const but the current implementation seems like it would allow it. Will check. Created attachment 348674 [details]
Patch
Comment on attachment 348674 [details] Patch Clearing flags on attachment: 348674 Committed r235570: <https://trac.webkit.org/changeset/235570> All reviewed patches have been landed. Closing bug. |