1023618 - The reftest harness should always call FlushRendering

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Always call FlushRendering in the reftest harness

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=481350a2b021

Comment 3

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb7f834871a

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4cb7f834871a

Comment 5

[Seth Fowler [:seth] [:s2h]]

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Whoops, removing that request, since it looks like this patch itself caused a new test failure. We need a followup for that before backporting.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (FOLLOWUP) - Ensure that win.document.documentElement exists before using it

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.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Try job:

https://tbpl.mozilla.org/?tree=Try&rev=36ba19c0e1bc

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 10

[Ryan VanderMeulen [: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 :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Everything looks good on Try, so I went ahead and pushed it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf3aebcaeeb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/866eb4e421b2
https://hg.mozilla.org/releases/mozilla-beta/rev/423cd6386bd2
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4038383d5d6a
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e1bb6e9a4853
https://hg.mozilla.org/releases/mozilla-esr24/rev/3d5d61210d6b

Comment 13

[Seth Fowler [:seth] [:s2h]]

(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!

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8cf3aebcaeeb