Bug 187389 – Layout Test editing/selection/navigation-clears-editor-state.html is flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Truitt Savell]

The following layout test is flaky on Sierra and High Sierra WK1

editing/selection/navigation-clears-editor-state.html

Probable cause:

I do not know, though it seems to have begun around revision r233184

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fselection%2Fnavigation-clears-editor-state.html

looking into issue further

[Ryan Haddad]

Saam attempted to fix this in https://trac.webkit.org/changeset/233555, it it failing after that revision?

[Truitt Savell]

Yes its still flakey after that revision. But it seems like his revision might have fixed the test for windows.

[Saam Barati]

Created attachment 344423 [details]
patch

[WebKit Commit Bot]

Comment on attachment 344423 [details]
patch

Clearing flags on attachment: 344423

Committed r233581: <https://trac.webkit.org/changeset/233581>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41898822>

[Truitt Savell]

So it looks like after that new revision the test is constantly failing on Sierra and High Sierra Debug WK1 and WK2.

example of failure message:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/r233581%20(4602)/editing/selection/navigation-clears-editor-state-pretty-diff.html

[Ryan Haddad]

Reopening per Truitt's comment.

[Ryosuke Niwa]

I think we just need to skip this test on debug bots. It's too slow there.

[Saam Barati]

(In reply to Ryosuke Niwa from comment #9)
> I think we just need to skip this test on debug bots. It's too slow there.

Agreed. I'll do this.

[Saam Barati]

(In reply to Saam Barati from comment #10)
> (In reply to Ryosuke Niwa from comment #9)
> > I think we just need to skip this test on debug bots. It's too slow there.
> 
> Agreed. I'll do this.

I spoke with Ryosuke offline. We're going to start by just removing the 18 second timeout.

[Saam Barati]

(In reply to Saam Barati from comment #11)
> (In reply to Saam Barati from comment #10)
> > (In reply to Ryosuke Niwa from comment #9)
> > > I think we just need to skip this test on debug bots. It's too slow there.
> > 
> > Agreed. I'll do this.
> 
> I spoke with Ryosuke offline. We're going to start by just removing the 18
> second timeout.

We're also going to mark it slow on Debug

[Saam Barati]

(In reply to Saam Barati from comment #12)
> (In reply to Saam Barati from comment #11)
> > (In reply to Saam Barati from comment #10)
> > > (In reply to Ryosuke Niwa from comment #9)
> > > > I think we just need to skip this test on debug bots. It's too slow there.
> > > 
> > > Agreed. I'll do this.
> > 
> > I spoke with Ryosuke offline. We're going to start by just removing the 18
> > second timeout.
> 
> We're also going to mark it slow on Debug

Actually, our new approach will be:
- lower iframe count from 200 to 100
- remove 18 second timer

We'll mark it as slow in the future if it times out.

[Saam Barati]

(In reply to Saam Barati from comment #13)
> (In reply to Saam Barati from comment #12)
> > (In reply to Saam Barati from comment #11)
> > > (In reply to Saam Barati from comment #10)
> > > > (In reply to Ryosuke Niwa from comment #9)
> > > > > I think we just need to skip this test on debug bots. It's too slow there.
> > > > 
> > > > Agreed. I'll do this.
> > > 
> > > I spoke with Ryosuke offline. We're going to start by just removing the 18
> > > second timeout.
> > 
> > We're also going to mark it slow on Debug
> 
> Actually, our new approach will be:
> - lower iframe count from 200 to 100
> - remove 18 second timer
> 
> We'll mark it as slow in the future if it times out.

Landed in:
https://trac.webkit.org/changeset/233701/webkit

[Truitt Savell]

After r233701 <https://trac.webkit.org/changeset/233701/webkit>

Failure is back to being Flaky on WK1 Release Platforms. It has stopped failing on Debug.

Example of test output on failure:

--- /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/editing/selection/navigation-clears-editor-state-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/editing/selection/navigation-clears-editor-state-actual.txt
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS freed more than 15% of documents
+FAIL freed less than 15% of documents
 PASS successfullyParsed is true
 
 TEST COMPLETE

Test History: 
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fselection%2Fnavigation-clears-editor-state.html

[Mark Lam]

(In reply to Truitt Savell from comment #15)
> After r233701 <https://trac.webkit.org/changeset/233701/webkit>
> 
> Failure is back to being Flaky on WK1 Release Platforms. It has stopped
> failing on Debug.
> 
> Example of test output on failure:
> 
> ---
> /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/
> editing/selection/navigation-clears-editor-state-expected.txt
> +++
> /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/
> editing/selection/navigation-clears-editor-state-actual.txt
> @@ -3,7 +3,7 @@
>  On success, you will see a series of "PASS" messages, followed by "TEST
> COMPLETE".
>  
>  
> -PASS freed more than 15% of documents
> +FAIL freed less than 15% of documents
>  PASS successfullyParsed is true
>  
>  TEST COMPLETE
> 
> Test History: 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=editing%2Fselection%2Fnavigation-clears-editor-
> state.html

How about adding <!-- webkit-test-runner [ jscOptions=--useConcurrentGC=false ] --> to the top of the test?  That should make the GCing more consistent.

[Saam Barati]

Ugh, we are putting more attention than needed into this test IMO. I say we just mark it as flaky and move on.

[Saam Barati]

(In reply to Mark Lam from comment #16)
> (In reply to Truitt Savell from comment #15)
> > After r233701 <https://trac.webkit.org/changeset/233701/webkit>
> > 
> > Failure is back to being Flaky on WK1 Release Platforms. It has stopped
> > failing on Debug.
> > 
> > Example of test output on failure:
> > 
> > ---
> > /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/
> > editing/selection/navigation-clears-editor-state-expected.txt
> > +++
> > /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/
> > editing/selection/navigation-clears-editor-state-actual.txt
> > @@ -3,7 +3,7 @@
> >  On success, you will see a series of "PASS" messages, followed by "TEST
> > COMPLETE".
> >  
> >  
> > -PASS freed more than 15% of documents
> > +FAIL freed less than 15% of documents
> >  PASS successfullyParsed is true
> >  
> >  TEST COMPLETE
> > 
> > Test History: 
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=editing%2Fselection%2Fnavigation-clears-editor-
> > state.html
> 
> How about adding <!-- webkit-test-runner [
> jscOptions=--useConcurrentGC=false ] --> to the top of the test?  That
> should make the GCing more consistent.

I believe the test is calling sync GCs manually. So I don't see how this will help

[Saam Barati]

We could also just bump up the iteration count a bit again

[Mark Lam]

(In reply to Saam Barati from comment #18)
> (In reply to Mark Lam from comment #16)
> > How about adding <!-- webkit-test-runner [
> > jscOptions=--useConcurrentGC=false ] --> to the top of the test?  That
> > should make the GCing more consistent.
> 
> I believe the test is calling sync GCs manually. So I don't see how this
> will help

You're right: GCController.collect() ultimately ends up in GCController::garbageCollectNow() in the web process.  So, this wouldn't help.

[Ryosuke Niwa]

I think this test is just slow. I think 200 was working fine. So let's go back there and just mark this test as SLOW.

[Ryosuke Niwa]

Created attachment 344816 [details]
Another fix attempt

[Ryosuke Niwa]

Committed r233754: <https://trac.webkit.org/changeset/233754>

[Ryosuke Niwa]

Looks like this latest change finally made it stable!

[Saam Barati]

(In reply to Ryosuke Niwa from comment #24)
> Looks like this latest change finally made it stable!

Great! Thanks Ryosuke