Closed Bug 1113425 Opened 5 years ago Closed 5 years ago

test_deferred_start.html fails on B2G desktop Linux opt

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(4 files)

Bug 927349 adds a test for compositor animations (dom/animation/test/mozilla/test_deferred_start.html) that seems to fail on B2G Desktop Linux opt. The animations don't seem to appear on the compositor in that case. I'm not sure what the difference is there. Other similar tests (e.g. dom/animation/test/chrome/test_running_on_compositor.html) are fine since they aren't run on that platform.

For now I'm marking the test as failing until I can investigate why.
I'm currently looking into this in bug 1119981 (which is probably a dupe of this).
See Also: → 1119981
The problem here is described in bug 1119981 comment 28.

Basically, we're not in test mode in this case so we don't get the forced synchronous composition operations that normally happen when we're in test mode. As a result, there are race conditions when the shadow layer tree gets updated.

The solution might be to factor out a "FlushPendingComposites" method in CompositorParent for testing purposes only and call that from LayerTransactionParent::RecvGetAnimationTransform (or even from getOMTAStyle?) That might even remove the need for the special handling in CompositorParent::ShadowLayersUpdated?
It turns out there's more to this than the race condition in comment 2 (although that needs to be fixed too).

This test is permafail on B2G Desktop (for at least Linux and Windows). I've been trying to debug why and so far I can see we never create the layer animation because the never make the nsDisplayTransform. I haven't worked out why yet.

We get to BuildDisplayListStackingContext but I think we're on the root frame there (mState & NS_FRAME_MAY_BE_TRANSFORMED returns false and we don't have any transform animations for that frame). We don't seem to get much further than that before we decide painting has finished and its time to start the animation for real.

It's possible there's something amiss about the way we detect when painting has finished but curiously this test passes on Mulet and on emulator builds. I wonder if it's related to the way we run tests on B2G desktop.

Matt, sorry to keep bothering you with these kind of questions, but any ideas what might be different about display list building on B2G desktop?
Flags: needinfo?(matt.woodrow)
It turns out this was due to the B2G desktop window being sized such that it has a 0x0 content area so we weren't painting at all. I'm not sure if only happens on Windows or if it affects other platforms too. Pushing a patch to try now with some debugging to check.
Flags: needinfo?(matt.woodrow)
That try run is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75ab28b311ee

Locally, I get the following output:
  Test window is (inner: 496 x 166), (outer: 16 x 39)

i.e. window.innerWidth/innerHeight is reporting a size larger than outerWidth/outerHeight. I guess somewhere we're initializing the viewport to 496x166 but when we paint we look at how much is actually being displayed.

Locally I've just added a 5s setTimeout call to the start of the test and resized the window before the test continues. If I do that, it passes.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Here's another try run which removes what is hopefully a redundant forced composite. I want to verify if this is actually unneeded by comparing the two runs.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3c9530f7cf
Looks like it is a window size problem.

On B2G desktop Linux on try I get:

> INFO -  Test window is (inner: 500 x 300), (outer: 320 x 510)
> INFO -  Performance warning: Async animation disabled because frame size (484, 100) is bigger than the viewport (360, 574) or the visual rectangle (484, 100) is larger than the max allowable value (17895698) [div]

But on Mulet I get:

> INFO -  Test window is (inner: 500 x 300), (outer: 1151 x 854)

And the test passes on Mulet.

I need to debug why we get a frame size of 484x100 back for a 100x100 div that is translated 100px.
(In reply to Brian Birtles (:birtles) from comment #7)
> I need to debug why we get a frame size of 484x100 back for a 100x100 div
> that is translated 100px.

I got part way into this (so far as to confirm that nsIFrame::FinishAndStoreOverflow is being called with new size 484x100 and old size 0x0) but I realised if I make the div position:absolute we seem to pass this check.
Wow, that's the most embarrassing bug of all. It turns out the reason we get a frame size of 484x100 is because the width declaration in the stylesheet was skipped because it was preceded by a // comment which CSS doesn't allow. 
In order to test off-main thread animations, we have a method that will return
the animated transform value set on a shadow layer. This method will return null
if the transform was not set by animation.

However, in some situations we temporarily clear the animation transform. For
example, when we synchronize a composite layer with its content layer, we reset
the animation transform. Then, on the next composite, we will recalculate the
animated value.

If we try to query the animated transform value inbetween resetting it and the
next composite we will get back null. To avoid a race condition, in
ShadowLayersUpdated after potentially clearing the animated transform, we force
a synchronous composite to reinstate the animated transform so it is there when
we go to query it. However we *only* do this when the mIsTesting flag is set
which is true whenever we have the refresh driver under test control.
Furthermore, we only do it when we already have a pending composite task to
better match conditions under regular operation.

In test_deferred_start.html, however, we specifically need to test without
putting the refresh driver under test control. As a result mIsTesting will be
false and we can encounter a race condition when querying the animated
transform.

To work around this, this patch makes us *also* flush pending composites when
fetching the animated transform value. The method for getting the animated
transform value is only used for testing so it should have no effect on the
regular compositing behavior.

It would seem that we could then remove the call from ShadowLayersUpdated but
doing this caused a small number of test cases to fail. In particular one test
for *opacity* in test_animations_omta.html was failing at the end of the
animation because we ended up with a stale opacity animation value on the
compositor which the synchronous composite was previously removing. The test,
in this case, should be ignoring the value on the compositor but, unlike
transform, there is no flag for indicating whether or not the opacity on shadow
layers has been set by animations. As a result, this patch leaves the call that
triggers a synchronous composite in test mode when updating shadow layers.
Attachment #8576980 - Flags: review?(matt.woodrow)
This is needed to avoid intermittent failures on Mulet and B2G ICS Emulator.
Attachment #8576981 - Flags: review?(matt.woodrow)
Attachment #8576979 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8576980 [details] [diff] [review]
part 2 - Flush pending composites when querying the animated transform

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1183,5 @@
>    mIsTesting = false;
>  }
>  
> +void
> +CompositorParent::FlushPendingComposites(LayerTransactionParent* aLayerTree)

This doesn't actually do any compositing as such, it just applies all async properties to the layer tree (and potentially fires the composited event back to the main thread). The name/comments are probably a bit misleading in that respect.
Attachment #8576980 - Flags: review?(matt.woodrow) → review+
Attachment #8576981 - Flags: review?(matt.woodrow) → review+
Attachment #8576982 - Flags: review?(matt.woodrow) → review+
I've updated some of the naming as suggested in comment 16.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=685ddf3a9bb1
See Also: → 1168759
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.