Bug 104951 – Layout Test editing/selection/move-by-character-crash-test-textarea.html is flaky

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

The following layout test is flaky on most platforms (at least cr-linux and cr-win)

editing/selection/move-by-character-crash-test-textarea.html
http://trac.webkit.org/browser/trunk/LayoutTests/editing/selection/move-by-character-crash-test-textarea.html

It appears that the notifications are racy with the completion of the test?

[Eric Seidel (no email)]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fselection%2Fmove-by-character-crash-test-textarea.html

[Yi Shen]

Looking at it
(In reply to comment #1)
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fselection%2Fmove-by-character-crash-test-textarea.html

[Yi Shen]

It seems I need to turn off the TestRunner's dumpEditingCallbacks flag, otherwise, it gives some extra output on some platform ...

EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
PASS

[Eric Seidel (no email)]

Seems reasonable.  It's possible that it turns it on automagically for you in editing.

[Yi Shen]

Created attachment 179372 [details]
fix the flaky test

[Eric Seidel (no email)]

I'm confused by this fix.  Could you please explain it better in the ChangeLog?

[Ryosuke Niwa]

Comment on attachment 179372 [details]
fix the flaky test

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

> LayoutTests/editing/selection/move-by-character-crash-test-textarea.html:-8
> -onload = function() {

Why does this help? DRT already waits for load event.

[Yi Shen]

Created attachment 179502 [details]
Fix the flaky test by turning on the dumpEditingCallbacks

[Eric Seidel (no email)]

Why does this help?  Wasn't the failure because we sometimes saw editing callbacks in this test?  Perhaps those were bleeding through from the test before this one?

[Yi Shen]

Eric, sorry for the confusion. I tried to turn off the dumpEditingCallbacks flag yesterday, but there is no such JS interface. This flag is suppose to remain off by default, however, it is turned on for some reason on some build. (Is it possible that we are using a global TestRunner?)

Then I checked other similar tests under the editing/selection, and found most of them don't use the onload function, but call their test function after the </body> tag. (Niwa, that's why I move the onload function). Since I don't have an environment to reproduce this flaky issue ( I have tried both my mac and linux build), I uploaded the patch and was hoping the contribution system can help me test it. But it seems it doesn't work in that way.

For the new patch, I turned on the dumpEditingCallbacks flag, then updated the expected file.  Please let me know if you think it is not a good idea to do so.

(In reply to comment #9)
> Why does this help?  Wasn't the failure because we sometimes saw editing callbacks in this test?  Perhaps those were bleeding through from the test before this one?

[Yi Shen]

I have same guess. But can't verify it since none of my builds can reproduce this flaky issue.
(In reply to comment #9)
>  Perhaps those were bleeding through from the test before this one?

[Ryosuke Niwa]

(In reply to comment #9)
> Why does this help?  Wasn't the failure because we sometimes saw editing callbacks in this test?  Perhaps those were bleeding through from the test before this one?

Yup.

12:21:06.552 1152 worker/2 editing/selection/move-by-character-6.html passed
12:21:06.609 1154 worker/3 fast/backgrounds/background-origin-root-element.html passed
12:21:06.768 1151 worker/1 http/tests/security/mixedContent/empty-url-plugin-in-frame.html passed
12:21:06.772 1162 worker/7 editing/style/style-text-node-without-editable-parent.html passed
12:21:06.838 1158 worker/5 fast/block/block-size-integer-overflow.html passed
12:21:06.857 1160 worker/6 fast/backgrounds/size/backgroundSize20.html passed
12:21:06.880 1134 [8074/26903] editing/selection/move-by-character-crash-test-textarea.html failed unexpectedly (text diff)
12:21:06.874 1152 worker/2 editing/selection/move-by-character-crash-test-textarea.html failed:
12:21:06.874 1152 worker/2  text diff

editing/selection/move-by-character-6.html sets commandDelay but never calls waitUntilDone or notifyDone.

[Ryosuke Niwa]

Created attachment 179546 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 179547 [details]
Reveted unrelated WebCore change

[Yi Shen]

You can remove the variable "commandCount", right? Because the queueCommand doesn't need second parameter anymore.

> queueCommand(execSetSelectionCommand.bind(execSetSelectionCommand, sn, so, en, eo), commandCount * commandDelay);

Also, I am still curious why move-by-character-6.html isn't flaky ...

(In reply to comment #14)
> Created an attachment (id=179547) [details]
> Reveted unrelated WebCore change

(In reply to comment #14)
> Created an attachment (id=179547) [details]
> Reveted unrelated WebCore change

[WebKit Review Bot]

Comment on attachment 179547 [details]
Reveted unrelated WebCore change

Attachment 179547 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15323859

New failing tests:
editing/selection/move-by-character-6.html

[Eric Seidel (no email)]

Comment on attachment 179547 [details]
Reveted unrelated WebCore change

Wow.  So this was just pooping all over lots of other tests?

[Eric Seidel (no email)]

Or just the one following test?  Don't we have code to kill timers before starting the next test?

[Ryosuke Niwa]

(In reply to comment #17)
> (From update of attachment 179547 [details])
> Wow.  So this was just pooping all over lots of other tests?

Probably not. Just one test.

[Eric Seidel (no email)]

Comment on attachment 179547 [details]
Reveted unrelated WebCore change

Is that the expected number of editing delegate log messages now?

[Ryosuke Niwa]

(In reply to comment #20)
> (From update of attachment 179547 [details])
> Is that the expected number of editing delegate log messages now?

Yes.

[Ryosuke Niwa]

Committed r137953: <http://trac.webkit.org/changeset/137953>