Closed Bug 1254831 Opened 4 years ago Closed 4 years ago

[e10s] Win8 TEST-UNEXPECTED-PASS in some mathml/mfrac reftests with e10s enabled

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: RyanVM, Assigned: jkitch)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Fred, I don't know these tests well enough to say whether the results are what we would expect or not, but they are permanently passing on Windows 8 when e10s is forced on. Any thoughts? :)

https://treeherder.mozilla.org/logviewer.html#?job_id=17687025&repo=try
Flags: needinfo?(fred.wang)
Flags: needinfo?(fred.wang)
Assignee: nobody → jkitch.bug
I can reproduce this on Windows 10.

Those tests have been problematic for a while.

Pass in bug 975681
Subsequently marked as random failure on Windows XP 
Marked as Perma-fail on Windows in bug 947654
Now only failing when e10s is disabled.

With e10s off, they fail because the fraction bar is not updated after an attribute change.

I'll have a closer look to see if I can identify any underlying issues, but having the tests start working is a good thing.
Is the rendering correct in the screenshots taken by the harness? If so, I'm happy to annotate this away. I just don't want to assume that passing is automatically the right thing here :)
Yes, rendering is correct.
Great, I'll go ahead and annotate this for now. I'm leaving the bug open under the assumption that you're intending to take a closer look at these tests based on comment 1. Feel free to remove the leave-open if that's not the case.
Keywords: leave-open
Backed out for https://hg.mozilla.org/integration/mozilla-inbound/rev/83b5406a8395 from comment 5 for failing on Windows 8 x64 opt.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6646425f62

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

REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/mathml/mfrac-B-2.html | image comparison (==), max difference: 255, number of differing pixels: 84
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/mathml/mfrac-B-3.html | image comparison (==), max difference: 255, number of differing pixels: 42
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/mathml/mfrac-C-2.html | image comparison (==), max difference: 255, number of differing pixels: 84
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/mathml/mfrac-D-2.html | image comparison (==), max difference: 255, number of differing pixels: 126
Flags: needinfo?(ryanvm)
Argh, pretty sure I had the new condition on the wrong side of a parenthesis.
http://images.memes.com/meme/702133
Flags: needinfo?(ryanvm)
I had a fun insight recently that's probably relevant here - Win8 is our only Windows platform that runs reftests with D2D enabled currently. I found that out when running Win10 reftests (currently not supported, but being worked on) and hitting the same failure. So more realistically, this is likely a D2D+e10s (or APZ maybe) issue.
Attached patch 1. PatchSplinter Review
Stepping through the code, reflow correctly picks up on the modified line thickness, and the modified fraction bar is added to the display list for painting, but the invalidation algorithm does not always detect that the rendering has changed. As a result, the region is not redrawn.

Explicitly invalidating the frame when linethickness changes is sufficient to resolve the problem.  

A cursory search on DXR found other layout/ AttributeChanged overrides performing the task as well.
Attachment #8736661 - Flags: review?(fred.wang)
Comment on attachment 8736661 [details] [diff] [review]
1.  Patch

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

Thanks.
Attachment #8736661 - Flags: review?(fred.wang) → review+
Rather trivial fix.

Some #ifdefed out code was not included in the DisplayList refactoring some time ago.

This makes it compile when SHOW_BOUNDING_BOX is defined.  No changes occur when it isn't defined.
Attachment #8736662 - Flags: review?(fred.wang)
Attachment #8736662 - Flags: review?(fred.wang) → review+
You need to log in before you can comment on or make changes to this bug.