Closed Bug 1023618 Opened 6 years ago Closed 6 years ago

The reftest harness should always call FlushRendering

Categories

(Testing :: Reftest, defect)

x86
macOS
defect
Not set

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed, firefox-esr24 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed
firefox-esr24 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

(Whiteboard: [qa-] )

Attachments

(2 files)

It looks like bug 617152 changed the behavior of the reftest harness such that FlushRendering is not always called.

Previously, OnDocumentLoad would invoke StartWaitingForTestEnd via setTimeout, and StartWaitingForTestEnd would call FlushRendering. In this revision, that changed:

http://hg.mozilla.org/mozilla-central/rev/19f780daecd0

Now we don't always end up calling WaitForTestEnd, because AfterOnLoadScripts takes an initial snapshot and sometimes decides that that initial snapshot is good enough. See here for the full criteria:

http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#606

This change looks like it could be causing intermittent oranges. In particular, it seems likely that it's responsible for bug 942004. ImageDocument calls SetWidth and SetHeight on an image element in a load event handler, and the reftest harness doesn't always wait for the consequent style flush and eventual layout flush before taking its snapshot.

Ensuring that we always call FlushRendering should be enough to fix this problem. An alternate fix would be to always call WaitForTestEnd, but that seemed a bit more heavyweight so I didn't do things that way for the initial version of the patch.
This patches ensures that FlushRendering is always called before the reftest harness takes its initial snapshot. To make FlushRendering available at that point in the code, the patch also floats it to the top level.
Attachment #8438042 - Flags: review?(roc)
Blocks: 617152, 942004
https://hg.mozilla.org/mozilla-central/rev/4cb7f834871a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1025042
Comment on attachment 8438042 [details] [diff] [review]
Always call FlushRendering in the reftest harness

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 617152
User impact if declined: None. This is a testing issue. Our sheriffs will appreciate having fewer intermittent oranges, though.
Testing completed (on m-c, etc.): Landed last week with no issues since.
Risk to taking this patch (and alternatives if risky): Zero risk, since this patch only affects testing.
String or IDL/UUID changes made by this patch: None.
Attachment #8438042 - Flags: approval-mozilla-beta?
Attachment #8438042 - Flags: approval-mozilla-aurora?
Attachment #8438042 - Flags: approval-mozilla-beta?
Attachment #8438042 - Flags: approval-mozilla-aurora?
Whoops, removing that request, since it looks like this patch itself caused a new test failure. We need a followup for that before backporting.
This patch is intended to fix bug 1025042. It seems like a fairly brute force way to avoid the problem, but it seems to me that:

(1) It's OK if the documentElement doesn't exist (probably because the page is already being torn down, I imagine). There isn't anything better we could do in such a case.
(2) In general (as far as I know), a child frame could have a missing document element, so it's not clear that we could just insert a check at the FlushRendering callsite.

roc, if you think another approach is better let me know.
Attachment #8441130 - Flags: review?(roc)
FYI, patches that only touch tests or the test harness don't require approval for uplift :). I'll make sure this gets everywhere it can reasonably go once it sticks :)
Flags: needinfo?(ryanvm)
(In reply to Seth Fowler [:seth] from comment #8)
> https://tbpl.mozilla.org/?tree=Try&rev=36ba19c0e1bc

Given that bug 1025042 hit on the B2G emulator, I would have thought this would have had such a run. I've gone ahead and triggered an emulator build for you :)
Flags: needinfo?(ryanvm)
Everything looks good on Try, so I went ahead and pushed it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf3aebcaeeb
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO June 19-22] from comment #10)
> Given that bug 1025042 hit on the B2G emulator, I would have thought this
> would have had such a run. I've gone ahead and triggered an emulator build
> for you :)

Heh, yes, that would have been a good idea. That's what I get for pushing in a hurry. Thanks for triggering the emulator run, and for the uplift!
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.