Closed Bug 1383512 Opened 7 years ago Closed 7 years ago

Intermittent dom/base/test/test_user_select.html | test1: selected text - got "aaaaaccccccc", expected "aaaaaacc"

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mantaroh)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:race])

Attachments

(2 files, 1 obsolete file)

Bug 1383512 - Skip test_user_select.html on windows of opt and pgo.
59 bytes, text/x-review-board-request
gbrown
: review+
Details
Bug 1383512 - Add waiting code for FontFaceSet.ready and requestAnimationFrame into test_user_select.html
1.49 KB, text/plain
mantaroh
: review+
Details 
Filed by: archaeopteryx [at] coole-files.de

https://treeherder.mozilla.org/logviewer.html#?job_id=116689768&repo=mozilla-inbound

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1500762019/mozilla-inbound_win7_vm_test-mochitest-e10s-1-bm140-tests1-windows-build309.txt.gz

17:35:22     INFO - TEST-START | dom/base/test/test_user_select.html
17:35:22     INFO - TEST-INFO | started process screenshot
17:35:22     INFO - TEST-INFO | screenshot: exit 0
17:35:22     INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | test1: selected text - got "aaaaaccccccc", expected "aaaaaacc"
17:35:22     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
17:35:22     INFO -     checkText@dom/base/test/test_user_select.html:107:5
17:35:22     INFO -     test@dom/base/test/test_user_select.html:153:3
17:35:22     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
17:35:22     INFO -     window.onload@dom/base/test/test_user_select.html:336:30
17:35:22     INFO -     EventHandlerNonNull*@dom/base/test/test_user_select.html:336:1
Priority: -- → P5
Hi Jet, is this something your team could take a look?
Component: DOM → Selection
Flags: needinfo?(bugs)
I think this test is racy because it depends on the Ahem font being loaded and used for the text fields, which are then selected using device pixel offsets. 

Switching it from window.onload to body.onload like this test, should help:
http://searchfox.org/mozilla-central/source/dom/base/test/test_caretPositionFromPoint.html#114
Mantaroh, would you mind looking into fixing this test?
Flags: needinfo?(bugs) → needinfo?(mantaroh)
(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #6)
> Mantaroh, would you mind looking into fixing this test?

OK. I will do that.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Flags: needinfo?(mantaroh)
:mantaroh - Are you making progress here? Unless a fix is coming soon, I'd like to disable the test while you work on it.
Flags: needinfo?(mantaroh)
I think that this phenomenon is timing issue. I couldn't reproduce this phenomenon on my laptop(win7VM / PGO build). But I confirmed that we can't reproduce this phenomenon by using following waiting code on try[1]:
 * wait window.load()
 * wait FontFace.load() for Ahem font.
 * wait requestAnimationFrame() for styling.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=391e2633bb9bf2d68cb78d9ff4c09803e886d894

(In reply to Geoff Brown [:gbrown] from comment #10)
> :mantaroh - Are you making progress here? Unless a fix is coming soon, I'd
> like to disable the test while you work on it.

OK. I'll disable this test on windows pgo and opt until clarify this reason.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eb04e77e4ba6611bda1f5a0490736e3f8960000
Flags: needinfo?(mantaroh)
Comment on attachment 8895227 [details]
Bug 1383512 - Skip test_user_select.html on windows of opt and pgo.

https://reviewboard.mozilla.org/r/166372/#review171700

Thanks!
Attachment #8895227 - Flags: review?(gbrown) → review+
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a17b61bc3e8
Skip test_user_select.html on windows of opt and pgo. r=gbrown
(In reply to Pulsebot from comment #14)
> Pushed by gbrown@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/3a17b61bc3e8
> Skip test_user_select.html on windows of opt and pgo. r=gbrown

Thanks Geoff for pushing it.

Added leave-open keyword. I'll continue to investigate this phenomenon.
Keywords: leave-open
Whiteboard: [stockwell needswork:DOM] → [stockwell disabled]
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> (In reply to Pulsebot from comment #14)
> > Pushed by gbrown@mozilla.com:
> > https://hg.mozilla.org/integration/autoland/rev/3a17b61bc3e8
> > Skip test_user_select.html on windows of opt and pgo. r=gbrown
> 
> Thanks Geoff for pushing it.
> 
> Added leave-open keyword. I'll continue to investigate this phenomenon.

I discussed with Brian.
Maybe, We will need to wait to the following event:

 * window.load : To load all resouces.
 * FontFaceSet.ready: To load all fonts.
 * requestAnimationFrame : To style.

Try :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8081278c422ffd5dfd2264f1cbd0e6b1c2aca31
Comment on attachment 8900997 [details] [diff] [review]
Bug 1383512 - Add waiting code for FontFaceSet.ready and requestAnimationFrame into test_user_select.html

># HG changeset patch
># Parent  f0abd25e1f4acced652d180c34b7c9eda638deb1
>Bug 1383512 - Add waiting code until loading FontSet and finishing styling to test_user_select.html. r?mats,birtles
>
>In some environment, test will start before applying font face.
>To ensure that content apply font, this patch changes to wait for starting test
>until loading FontSet and finishing styling.
>
>MozReview-Commit-ID: 25AqGDwQfBt
>
>diff --git a/dom/base/test/mochitest.ini b/dom/base/test/mochitest.ini
>--- a/dom/base/test/mochitest.ini
>+++ b/dom/base/test/mochitest.ini
>@@ -785,7 +785,7 @@ skip-if = debug == true && toolkit == 'a
> support-files = file_title.xul
> [test_treewalker_nextsibling.xml]
> [test_user_select.html]
>-skip-if = (toolkit == 'android') || (os == 'win' && !debug) # Windows is timing dependent(bug 1383512)
>+skip-if = toolkit == 'android'
> [test_viewport_scroll.html]
> [test_viewsource_forbidden_in_object.html]
> [test_w3element_traversal.html]
>diff --git a/dom/base/test/test_user_select.html b/dom/base/test/test_user_select.html
>--- a/dom/base/test/test_user_select.html
>+++ b/dom/base/test/test_user_select.html
>@@ -333,7 +333,18 @@ function test()
>   clear();
>   SimpleTest.finish();
> }
>-window.onload = function() { setTimeout(test, 0); };
>+
>+// Wait to window.onload() / FontFaceSet.ready() / requestAnimationFrame
>+// in order to ensure that load font resources and style it.
>+window.onload = function() {
>+  document.fonts.ready.then(function() {
>+    requestAnimationFrame(function() {
>+      test();
>+    });
>+  });
>+};
>+
>+
> SimpleTest.waitForExplicitFinish();
> </script>
> </pre>
Attachment #8900997 - Attachment is patch: true
Comment on attachment 8900997 [details] [diff] [review]
Bug 1383512 - Add waiting code for FontFaceSet.ready and requestAnimationFrame into test_user_select.html

Review of attachment 8900997 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following comments addressed.

(Not sure you need two reviewers for this.)

::: dom/base/test/test_user_select.html
@@ +334,5 @@
>    SimpleTest.finish();
>  }
> +
> +// Wait to window.onload() / FontFaceSet.ready() / requestAnimationFrame
> +// in order to ensure that load font resources and style it.

// These tests depend on the Ahem font being loaded and rendered so wait for
// fonts to load, then wait a frame for them to be rendered too.

@@ +335,5 @@
>  }
> +
> +// Wait to window.onload() / FontFaceSet.ready() / requestAnimationFrame
> +// in order to ensure that load font resources and style it.
> +window.onload = function() {

I'm curious, do we actually need to wait for onload here or is waiting for fonts enough?

@@ +339,5 @@
> +window.onload = function() {
> +  document.fonts.ready.then(function() {
> +    requestAnimationFrame(function() {
> +      test();
> +    });

Replace:

  requestAnimationFrame(function() {
    test();
  });

with:

  requestAnimationFrame(test);

@@ +343,5 @@
> +    });
> +  });
> +};
> +
> +

Nit: No need to two blank lines here.
Attachment #8900997 - Flags: review?(bbirtles) → review+
Thank you for reviewing it.

I think that this test need to wait for window.onload in order to prevent catch the callback which unrelated rendering. If removed this code, test was fail.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e89cdd99447a94020a7a09b642e9c766df05f74&group_state=expanded

I fixed this test.
Attachment #8900997 - Attachment is obsolete: true
Attachment #8900997 - Flags: review?(mats)
Attachment #8901024 - Flags: review+
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e88f7a90eea
Add waiting code until loading FontSet and finishing styling to test_user_select.html. r=birtles
Whiteboard: [stockwell disabled] → [stockwell fixed:race]
Mantaroh-san, I think you can drop the leave-open flag now and close this bug, right?
Flags: needinfo?(mantaroh)
(In reply to Brian Birtles (:birtles) from comment #26)
> Mantaroh-san, I think you can drop the leave-open flag now and close this
> bug, right?

Oh, Sorry.
I forgot to clear this flag.

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(mantaroh)
Keywords: leave-open
Resolution: --- → FIXED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: