Closed Bug 1090555 Opened 10 years ago Closed 9 years ago

Intermittent test_animations_omta.html | initial visited link background color - got rgb(255, 255, 0), expected rgb(0, 0, 255) | visited link background color after animation-only flush - got rgb(255, 255, 0), expected rgb(0, 0, 255)

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: RyanVM, Assigned: dbaron)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Fix visited link test in test_animations_omta.html to wait for visited link coloring properly
6.61 KB, patch
birtles
: review+
Details | Diff | Splinter Review 
Brian, looks like you're the primary owner of this test?

09:23:54     INFO -  889 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | number of events received for events at end of animation with newly empty keyframes rule
09:23:54     INFO -  890 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].type for events at end of animation with newly empty keyframes rule
09:23:54     INFO -  891 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].target for events at end of animation with newly empty keyframes rule
09:23:54     INFO -  892 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].animationName for events at end of animation with newly empty keyframes rule
09:23:54     INFO -  893 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].elapsedTime for events at end of animation with newly empty keyframes rule received=1 expected=1
09:23:54     INFO -  894 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].pseudoElement for events at end of animation with newly empty keyframes rule
09:23:54     INFO -  895 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | Animation is animating on compositor - got matrix(1, 0, 0, 1, 50, 0), expected matrix(1, 0, 0, 1, 50, 0)
09:23:54     INFO -  896 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | Animation is animating on compositor: OMTA style and computed style should be equal - OMTA matrix(1, 0, 0, 1, 50, 0), computed matrix(1, 0, 0, 1, 50, 0)
09:23:54     INFO -  897 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | Animation updated to use empty keyframes rule is cleared from compositor - got none, expected matrix(1, 0, 0, 1, 0, 0)
09:23:54     INFO -  898 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | number of events received for events at start of animation updated to use empty keyframes rule
09:23:54     INFO -  899 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].type for events at start of animation updated to use empty keyframes rule
09:23:54     INFO -  900 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].target for events at start of animation updated to use empty keyframes rule
09:23:54     INFO -  901 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].animationName for events at start of animation updated to use empty keyframes rule
09:23:54     INFO -  902 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].elapsedTime for events at start of animation updated to use empty keyframes rule received=0 expected=0
09:23:54     INFO -  903 INFO TEST-PASS | /tests/layout/style/test/test_animations_omta.html | events[0].pseudoElement for events at start of animation updated to use empty keyframes rule
09:23:54     INFO -  904 INFO TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_animations_omta.html | initial visited link background color - got rgb(255, 255, 0), expected rgb(0, 0, 255)
09:23:54     INFO -  905 INFO TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_animations_omta.html | visited link background color after animation-only flush - got rgb(255, 255, 0), expected rgb(0, 0, 255)
09:23:54     INFO -  906 INFO TEST-OK | /tests/layout/style/test/test_animations_omta.html | took 65982ms
Flags: needinfo?(birtles)
We shouldn't be turning OMTA on for Linux yet. It's not ready. At very least we should mark this test as failing for Mulet but I wonder what changed on fx-team/b2g-inbound recently to trigger this.
Flags: needinfo?(birtles)
I can't determine what regressed this. I'll wait until it hits m-c and hope that I can determine what changeset triggered it there.
This test also fails on Linux mochitest-e10s and on Android mochitest when I push a try run enabling OMT Animations.
Blocks: enable-omt-animations
https://hg.mozilla.org/mozilla-central/file/38154607d807/layout/style/test/test_animations_omta.html#l2036 has:

  // Wait for animations to start and for visited link coloring.
  yield waitForPaintsFlushed();

yet I don't think there's anything in:

https://hg.mozilla.org/mozilla-central/file/38154607d807/testing/mochitest/tests/SimpleTest/paint_listener.js

that waits for visited link coloring.


I don't think it's surprising that with e10s, visited link coloring might take longer than starting animations.
... which shows that my fix works on Linux M-e10s and on B2G, but not on Android.  (Maybe Android link coloring doesn't necessarily return results in the order requested?  That said... it's the same URL, so I'd have thought that wouldn't matter.)
er, I guess there was nothing to "work" on B2G, so really just works on Linux M-e10s.
Er, and I should have looked more closely; the Android failure is now (with the patch) the test timing out.
An earlier try run from today before comment 56:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa5e8dd1358f

Then, replacing setting href to "" with setting href to window.top.location.href seems to fix Android, probably because we run mochitests differently there (and I tested locally that it also still fixes Linux M-e10s):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c03ecf24c644
This patch contains two changes:
 (1) The addition of refVisitedLink and the use of
     waitForVisitedLinkColoring() on it.
 (2) Changing the URL of the visited lisks (both visitedLink and
     refVisitedLink) from "" to window.top.location.href, since the
     former doesn't work for Android mochitests while it does work on
     Linux mochitest-e10s.

I tested locally that without the patch I get the failures, and with the
patch the failures go away, using:
./mach mochitest-plain --e10s --setpref layers.acceleration.force-enabled=true --setpref layers.offmainthreadcomposition.async-animations=true layout/style/test/test_animations_omta.html

Further, when running (and passing), I checked that
waitForVisitedLinkColoring() does go through one setTimeout cycle.

Also, I tested that if I effectively revert
https://hg.mozilla.org/mozilla-central/rev/d13154302d77 by changing the
third parameter to the GetContext call in
nsStyleSet::ResolveStyleWithReplacement to be nullptr instead of
visitedRuleNode, I get the failure:
TEST-UNEXPECTED-FAIL | layout/style/test/test_animations_omta.html | visited link background color after animation-only flush - got rgb(255, 255, 0), expected rgb(0, 0, 255)
which confirms that the test is still testing what it was designed to
test.
Attachment #8582712 - Flags: review?(bbirtles)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Comment on attachment 8582712 [details] [diff] [review]
Fix visited link test in test_animations_omta.html to wait for visited link coloring properly

Thanks for the very helpful description (and especially for confirming that the test still fails if we revert the change it is testing)!

r=birtles with the following (mostly cosmetic) comments addressed as you see appropriate

>+function waitForVisitedLinkColoring(visitedLink, waitProperty, waitValue) {
>+  function check_link(resolve) {

Nit: Any reason to prefer check_link over checkLink?

>+    if (SpecialPowers.DOMWindowUtils
>+          .getVisitedDependentComputedStyle(visitedLink, "", waitProperty) ==
>+        waitValue) {
>+      // Our link has been styled as visited.  Resolve.
>+      resolve(true);
>+    } else {
>+      // Our link is not yet styled as visited.  Poll for completion.
>+      setTimeout(check_link, 0, resolve);

Nit: Should we be using SimpleTest.executeSoon here?

>diff --git a/layout/style/test/test_animations_omta.html b/layout/style/test/test_animations_omta.html
...
>   var bgColor = SpecialPowers.DOMWindowUtils
>     .getVisitedDependentComputedStyle(visitedLink, "", "background-color");
>-  var isb2g = SpecialPowers.Services.appinfo.name == "B2G";
>-  // No global history in B2G.
>-  (isb2g ? todo_is : is)(bgColor, "rgb(0, 0, 255)",
>-                         "initial visited link background color");
>+  is(bgColor, "rgb(0, 0, 255)", "initial visited link background color");
> 
>   if (isb2g) {
>     // The above failure makes the rest of the test pointless.
>     div1.remove();
>     visitedLink.remove();
>     return;
>   }

I think we can drop this last block here since we already have an early-return for isb2g at the top of this function now.
Attachment #8582712 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #59)
> Nit: Should we be using SimpleTest.executeSoon here?

I think not, since we actually want the timer throttling that setTimeout has.  (I'd actually have used 10 rather than 0 if that didn't trigger errors in mochitest.)
https://hg.mozilla.org/mozilla-central/rev/d96242aad96d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: