Closed Bug 1442063 Opened 6 years ago Closed 6 years ago

Don't do FlushRendering() in MakeProgress once we reached the state of STATE_WAITING_TO_FINISH

Categories

(Testing :: Reftest, enhancement)

Product:
Testing
Component:
Reftest
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file)

Bug 1442063 - Don't call FlushRendering() after we reached the state of STATE_WAITING_TO_FINISH.
59 bytes, text/x-review-board-request
dbaron
: review+
Details 
MakeProgress() calls FlushRendering() every time, but as far as I can tell, once we reached STATE_WAITING_TO_FINISH, we don't need to flush styles any more, we should just wait for the final MozAfterPaint.  If there were still pending styles, it's a flaw of the test case. 

On WebRender, we do fire MozAfterPaint even if there is no invalidation change.  So if the test case has off-main-thread animations, we do repeatedly fire MozAfterPaint event, so there is no chance to finish the reftest.  In other words, reftest on current Gecko works fine because we don't fire MozAfterPaint when no invalidation changes.   I did actually confirm that, even on current Gecko, the reftest has infinite "rotate(0deg) -> rotate(360deg)" animation does not finish.  Such test cases is ridiculous though. 

Here is a try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67266c6c280b22fbde0c3102c51a96557f623964
dbaron review ping
Flags: needinfo?(dbaron)
Comment on attachment 8954970 [details]
Bug 1442063 - Don't call FlushRendering() after we reached the state of STATE_WAITING_TO_FINISH.

https://reviewboard.mozilla.org/r/224158/#review231764

Clearing review request since the patch doesn't appear to be complete (see below).

Also, are there possible ways that this could invalidate tests by causing them to still pass, but no longer be testing the thing they were supposed to test?

::: layout/reftests/css-animations/reftest.list:71
(Diff revision 1)
>  
>  == continuation-opacity.html continuation-opacity-ref.html
>  == ib-split-sibling-opacity.html about:blank
>  
>  == opacity-animation-in-fixed-opacity-parent.html opacity-animation-in-fixed-opacity-parent-ref.html
> +== a.html about:blank

The "a.html" file isn't in this patch, and also isn't in the tree.  It also seems like the file could use a better name.

::: layout/tools/reftest/reftest-content.js:585
(Diff revision 1)
>          if (state >= STATE_COMPLETED) {
>              LogInfo("MakeProgress: STATE_COMPLETED");
>              return;
>          }
>  
> +        // We don't need to flush styles any more when we are the state waiting

"we are the state" -> "we are in the state"
Attachment #8954970 - Flags: review?(dbaron)
Thanks for the review!

a.html was slipped into the change somehow..

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #3)
> Also, are there possible ways that this could invalidate tests by causing
> them to still pass, but no longer be testing the thing they were supposed to
> test?

I don't think this patch breaks such test cases since this patch just stops a FlushRendering() call after it has called *after* reftest-wait was removed, so it's a redundant one as far as I can tell.  If the test cases suppose that the redundant call is necessary, it's odd.  But yes, it might be possible that if the test cases do odd things, for example what I can think of is that changing styles in a later tick after reftest-wait was removed (there might be saner examples though), so it's worth checking test cases anyway.

That's being said, I haven't written such invalidation checking tests though, your comment makes me think that WebRender should do invalidation check somehow for such test cases.  :kats what do you think?
Flags: needinfo?(bugmail)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> I don't think this patch breaks such test cases since this patch just stops
> a FlushRendering() call after it has called *after* reftest-wait was
> removed, so it's a redundant one as far as I can tell.  If the test cases
> suppose that the redundant call is necessary, it's odd.

This seems reasonable to me.

> But yes, it might
> be possible that if the test cases do odd things, for example what I can
> think of is that changing styles in a later tick after reftest-wait was
> removed (there might be saner examples though), so it's worth checking test
> cases anyway.

If a test changed styles after reftest-wait was removed, and expected the style change to show up in the snapshot, then your patch as it is would make that test fail. But the try push shows no such failures. I think the scenario dbaron is concerned about is if a test makes a change to a style after reftest-wait was removed, but is expecting the style change to have *no visual effect*. In that case, with current m-c, the extra FlushRendering() would flush that style change, and the snapshot would pick up any visual effects from that style change. But with your patch, that style change would never get flushed, so if we regressed behaviour such that the style change *does* cause a visual effect, the test would continue passing even though it's supposed to be failing.

However, I agree that if a test relied on this behaviour it would be very odd indeed. Per the reftest-wait documentation [1] the snapshot will be taken as soon as the reftest-wait is removed, so if the test is making changes to styles after reftest-wait is removed the test is relying on undefined behaviour in a really strange way, because it could just make the style change before removing reftest-wait and it would accomplish the exact same thing. So I think it's very unlikely we have tests that do this, except if they were written that way by accident.

> That's being said, I haven't written such invalidation checking tests
> though, your comment makes me think that WebRender should do invalidation
> check somehow for such test cases.  :kats what do you think?

I don't understand what you mean here, exactly. I'm assuming that by "invalidation checking tests" you mean the type of test I described above, which is the type of test dbaron is concerned about. And that you're looking for a way to detect these tests using WebRender? I don't think WebRender does anything special in this respect, that it would be able to detect such tests. I think if we want to detect such tests we would have to do a try push where that last FlushRendering() (the one you are removing) triggers an assertion if it encounters style changes that need flushing. If the assertion is tripped, then we know there is a test that is making style changes after the reftest-wait was removed, and that's bad.

[1] https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/layout/tools/reftest/README.txt#472
Flags: needinfo?(bugmail)
I am sorry for the confusion.  What I was thinking is an opposed example I mentioned.  That is, a reftest changes a style but the style change results no invalidation change, and then the reftest expects there is no invalidation change happened.  I don't know this example is truly a valid reftest and I don't know we can write such test case as reftest.  Anyway it's bit another story.

So, as for this bug, I think we should write a reftest having an extra style change, which fails without this fix and passes with the fix.  (And I guess dbaron suggested it in the comment?)

Anyway here is a try with the test case that works as expected locally.  Let's see the results on try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebe345146b5072bf50d174454ce1cab47e93b6b1
The reftest failed frequently. :/  We should match what the reftest harness does, i.e. making an extra style change in a callback of setTimeout() which is called inside a callback of MozAfterPaint [1].  Even with the setTimeout(), the reftest still fails without this fix locally.

After the attempt to write a reftest for this bug, it turns out it's really hard to sneak something which makes style changes into there even if we skip calling FlushRendering() in the state of STATE_WAITING_TO_FINISH.

I hope no more failure happens there.
https://hg.mozilla.org/try/rev/b2ed8c8909d71f9463ad848a67078d01dad17fe4

[1] https://searchfox.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#532-544
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> After the attempt to write a reftest for this bug, it turns out it's really
> hard to sneak something which makes style changes into there even if we skip

not to sneak, of course.

Unfortunately test case seems to be a perma failure on Quantum Render, and intermittent failure (only on stylo-disabled?).  So I am going to drop the test case here, but the test case convinced me that the patch does the right thing.
So this just needs an r+ then?
Comment on attachment 8954970 [details]
Bug 1442063 - Don't call FlushRendering() after we reached the state of STATE_WAITING_TO_FINISH.

https://reviewboard.mozilla.org/r/224158/#review234036
Attachment #8954970 - Flags: review?(dbaron) → review+
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20b2d3c8f26c
Don't call FlushRendering() after we reached the state of STATE_WAITING_TO_FINISH. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/20b2d3c8f26c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: