797942 - LayerManagerOGL not correctly updated on SIZE_CHANGED

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Geoff Brown [:gbrown]]

In bug 792212, we experience severe reftest failures occasionally on some tegras, and almost-always on other tegras. In the failure runs, nearly all the tests fail, because the screen is blank and remains blank for the duration of the test run.

Some robocop intermittent test failures have very similar symptoms (bug 767215) but have not been conclusively investigated.

Debugging the reftest failures shows that LayerManagerOGL::Render returns early because the width and height derived from mSurfaceSize are both 0. In the failure cases, the CompositorParent is created with a 0-size rect. nsWindow later receives a SIZE_CHANGED event with the correct size, and propagates the size change to windows, but not to the CompositorParent or LayerManagerOGL.

It appears that if SIZE_CHANGED is received after the CompositorParent is created, the CompositorParent and LayerManagerOGL continue to operate with 0-size bounds. I believe that resuming the compositor would rectify the situation, but in the case I am debugging, there is no compositor pause/resume.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: straw man / patch for discussion

Here's a terribly naive approach that appears to fix the reftest failures. I have hardly tested it otherwise. Is it okay to resume the compositor without pausing it?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 668081 [details] [diff] [review]
straw man / patch for discussion

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

::: widget/android/nsWindow.cpp
@@ +1247,5 @@
>      if (mWidgetListener) {
>          mWidgetListener->WindowResized(this, aSize.width, aSize.height);
>      }
> +
> +    ScheduleResumeComposition(aSize.width, aSize.height);

should we pause composition first to make sure the width and height don't change at an inopportune time?

Comment 3

[Geoff Brown [:gbrown]]

Attachment: on SIZE_CHANGED, pause then resume compositor with new size

I decided it would be better to pause first. 

I verified that this fixes the reftest problem and a try run looks fine: https://tbpl.mozilla.org/?tree=Try&rev=50684e431314

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 668267 [details] [diff] [review]
on SIZE_CHANGED, pause then resume compositor with new size

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

r+ with the commend addressed (just move the pause to before the mBounds modification).

::: widget/android/nsWindow.cpp
@@ +1247,5 @@
>      if (mWidgetListener) {
>          mWidgetListener->WindowResized(this, aSize.width, aSize.height);
>      }
> +
> +    SchedulePauseComposition();

I think it'd make sense to schedule the pause before resizing the widget.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: on SIZE_CHANGED, pause then resume the compositor with new size

Pause moved as per review comment. r=cwiiis

Comment 6

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a10b14ff06

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a7a10b14ff06

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think we should back this out and find another solution. The current patch doesn't even really make sense to me, given that GeckoLayerClient::surfaceChanged already sends a resume event (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/GeckoLayerClient.java#641). The patch we have in this bug is really causing some bad threading problems for Flash. (Bug 801627)

Comment 9

[Geoff Brown [:gbrown]]

But there are other code paths (besides surfaceChanged) that bring about a change to nsWindow bounds, and when that happens, surely we want to propagate the change to CompositorParent, etc. - wouldn't you say?

In the failed reftest (and possibly other tests) cases, I wonder if we are missing a surfaceChanged call, or if maybe the resize is lost because the compositor is not yet created? http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/GeckoLayerClient.java#628

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Geoff Brown [:gbrown] from comment #9)
> But there are other code paths (besides surfaceChanged) that bring about a
> change to nsWindow bounds, and when that happens, surely we want to
> propagate the change to CompositorParent, etc. - wouldn't you say?

I'm not 100% certain, but it looks to me like we only send it when the surface size changes.

> 
> In the failed reftest (and possibly other tests) cases, I wonder if we are
> missing a surfaceChanged call, or if maybe the resize is lost because the
> compositor is not yet created?

It looks like it's possible to miss a surfaceChanged call if the LayerView is added enough time before GeckoLayerClient.notifyGeckoReady is called (which hooks up the surfaceChanged listener). We send an initial resize event when the GeckoLayerClient is hooked up, but maybe it should send a compositor resume as well?

Comment 11

[Alex Keybl [:akeybl]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> The patch we have in this bug is really causing
> some bad threading problems for Flash. (Bug 801627)

Bug 801627 seems like enough reason to back this out until we find a fix.

Comment 12

[Geoff Brown [:gbrown]]

I checked with :callek today -- reftests are still not being assigned to new tegras, so backing this out should not cause many new test failures. I will look into the alternative suggested in Comment 10.

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/907354856a07

Comment 13

[Phil Ringnalda (:philor)]

Looks like Armv6 is particularly prone to the all-white reftests - of the 7 runs that have finished since your backout, 4 were all-white.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Geoff Brown [:gbrown] from comment #12)
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/907354856a07

https://hg.mozilla.org/mozilla-central/rev/907354856a07

Comment 15

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #12)
> I checked with :callek today -- reftests are still not being assigned to new
> tegras, so backing this out should not cause many new test failures. I will
> look into the alternative suggested in Comment 10.
> 
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/907354856a07

Shouldn't the (still applied AFAIK) workaround in bug 792212 be preventing this problem from reoccuring, still?

Comment 16

[Phil Ringnalda (:philor)]

There were two that I starred in bug 784278 during the period when that was in but this wasn't yet, dunno how many others there were that other people (or for that matter that I) just starred as "a" without mentioning it in any bug.

Anyway, the failure rate since this was backed out is 44%, all Android reftests on trunk trees are now hidden, bug 803408.

Comment 17

[Geoff Brown [:gbrown]]

(In reply to William Lachance (:wlach) from comment #15)
> (In reply to Geoff Brown [:gbrown] from comment #12)
> > I checked with :callek today -- reftests are still not being assigned to new
> > tegras, so backing this out should not cause many new test failures. I will
> > look into the alternative suggested in Comment 10.
> > 
> > Backed out:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/907354856a07
> 
> Shouldn't the (still applied AFAIK) workaround in bug 792212 be preventing
> this problem from reoccuring, still?

I thought/hoped that the bug 792212 workaround would continue to be effective. But I believe it worked by changing the timing of our startup...and that will tend to change as the code evolves.

Comment 18

[Geoff Brown [:gbrown]]

Following up on Comment 10...

Calling compositionResumeRequested in GeckoLayerClient.notifyGeckoReady is often ineffective because the compositor is not yet created (mCompositorCreated is false).

I thought it would be okay to call compositionResumeRequested in GeckoLayerClient.compositorCreated, but that does not work either -- the CompositorParent surface size is not updated. I don't know why yet.

Comment 19

[Geoff Brown [:gbrown]]

Calling compositionResumeRequested from GeckoLayerClient.compositorCreated fails because nsWindow.sCompositorParent is null -- nsWindow.SetCompositor has not yet been called. I'm looking for a better place (later in initialization) to update the compositor...open to suggestions!

Comment 20

[Geoff Brown [:gbrown]]

Attachment: update CompositorParent bounds from nsWindow on initialization

Here's an alternative solution: Instead of using SIZE_CHANGED to propagate bounds to the CompositorParent, do it in nsWindow::SetCompositor. This is the first chance to update the CompositorParent from nsWindow, and the nsWindow already has the correct bounds. It seems appropriate to sync-up the bounds with the new CompositorParent at this point. 

nsWindow::SetCompositor seems to be called multiple times during Fennec startup; this patch calls ScheduleResumeCompositor only on the first such call. Given the trouble with the previous patch, I'm trying to be conservative: propagate the bounds only when needed to get the reftests passing.

snorp: Can you verify that this patch does not cause trouble with full-screen flash?

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 673472 [details] [diff] [review]
update CompositorParent bounds from nsWindow on initialization

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

This looks ok to me, but I'm really not sure I'm the best person to review this anymore - Let's see what snorp says.

::: widget/android/nsWindow.cpp
@@ +2241,5 @@
>  nsWindow::SetCompositor(mozilla::layers::CompositorParent* aCompositorParent,
>                          mozilla::layers::CompositorChild* aCompositorChild)
>  {
> +    bool firstSetting = false;
> +    if (!sCompositorParent && aCompositorParent) {

Could/should this be sCompositorParent != aCompositorParent?

Comment 22

[Geoff Brown [:gbrown]]

(In reply to Chris Lord [:cwiiis] from comment #21)
> ::: widget/android/nsWindow.cpp
> @@ +2241,5 @@
> >  nsWindow::SetCompositor(mozilla::layers::CompositorParent* aCompositorParent,
> >                          mozilla::layers::CompositorChild* aCompositorChild)
> >  {
> > +    bool firstSetting = false;
> > +    if (!sCompositorParent && aCompositorParent) {
> 
> Could/should this be sCompositorParent != aCompositorParent?

No: If aCompositorParent == 0, we do not want to resize. Possibly aCompositorParent && (sCompositorParent != aCompositorParent) ...

Comment 23

[Geoff Brown [:gbrown]]

Unfortunately, try runs show that this patch is not always effective:

https://tbpl.mozilla.org/?tree=Try&rev=575957e7324c&noignore=1

Comment 24

[Kartikaya Gupta (email:[e-mail])]

I think you want to be setting the size on it whenever aCompositorParent != 0, not just on the "firstSetting". I'm also curious to know why it's getting set multiple times, if you can provide backtraces of the different invocations of that functions that might be useful.

Comment 25

[Geoff Brown [:gbrown]]

(In reply to Kartikaya Gupta (:kats) from comment #24)
> I think you want to be setting the size on it whenever aCompositorParent !=
> 0, not just on the "firstSetting". 

Good idea but that doesn't work either: https://tbpl.mozilla.org/?tree=Try&rev=d3e4a537874f&noignore=1

Comment 26

[Geoff Brown [:gbrown]]

(In reply to Kartikaya Gupta (:kats) from comment #24)
> I'm also curious to know why it's getting
> set multiple times, if you can provide backtraces of the different
> invocations of that functions that might be useful.

I'm sure I was originally seeing SetCompositor called 5 times at startup, but now it is only 3 times:
 ~nsWindow calls SetCompositor(NULL, NULL)
 ~nsWindow calls SetCompositor(NULL, NULL)
 nsWindow::GetLayerManager calls SetCompositor(mCompositorParent, mCompositorChild) after CreateCompositor()

I am a little disturbed by the idea of ~nsWindow calling SetCompositor and thereby resetting static members...but I haven't seen it cause a problem.

Comment 27

[Geoff Brown [:gbrown]]

Attachment: update CompositorParent bounds from nsWindow on initialization

There are 2 conditions necessary before the CompositorParent can have its size set correctly from nsWindow:
 - the CompositorParent must be created and set in nsWindow: nsWindow::SetCompositor must have been called to set sCompositor
 - nsWindow must know the window size: gAndroidBounds must be set

The test failures with the previous patch came about when SetCompositor was called before nsWindow had processed a SIZE_CHANGED event -- so gAndroidBounds was still (0,0) and the CompositorParent was updated with 0 size.

This patch allows for either SetCompositor or SIZE_CHANGED to occur first and only proceeds to update the CompositorParent size once both sCompositorParent and gAndroidBounds have been set

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 674229 [details] [diff] [review]
update CompositorParent bounds from nsWindow on initialization

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

I like this a lot better than the first patch and your explanation makes sense.

::: widget/android/nsWindow.cpp
@@ +2250,5 @@
>                          mozilla::layers::CompositorChild* aCompositorChild)
>  {
> +    bool sizeChangeNeeded = false;
> +    if (aCompositorParent && !sCompositorParent && gAndroidBounds.width != 0) {
> +        sizeChangeNeeded = true;

You can just do this all on one line:

bool sizeChangeNeeded = aCompositorParent && !sCompositorParent && gAndroidBounds.width != 0

Comment 29

[Kartikaya Gupta (email:[e-mail])]

(In reply to Geoff Brown [:gbrown] from comment #26)
> (In reply to Kartikaya Gupta (:kats) from comment #24)
> > I'm also curious to know why it's getting
> > set multiple times, if you can provide backtraces of the different
> > invocations of that functions that might be useful.
> 
> I'm sure I was originally seeing SetCompositor called 5 times at startup,
> but now it is only 3 times:
>  ~nsWindow calls SetCompositor(NULL, NULL)
>  ~nsWindow calls SetCompositor(NULL, NULL)
>  nsWindow::GetLayerManager calls SetCompositor(mCompositorParent,
> mCompositorChild) after CreateCompositor()
> 

Ah, thanks for digging into this. The fact that we create a bunch of nsWindows on startup concerns me quite a bit (I've run into this behaviour before and tried investigating it but still don't know why we do it). It's not part of this bug, but good to know that's why SetCompositor was getting called.

> I am a little disturbed by the idea of ~nsWindow calling SetCompositor and
> thereby resetting static members...but I haven't seen it cause a problem.

Yeah, I think there's a bunch of code that assumes there will be only one nsWindow object, but in practice I think once we're started up there are 3 or 4 that remain in existence during the Fennec lifetime. There are a couple that are created and destroyed during startup too, as you noticed.

> Comment on attachment 674229 [details] [diff] [review]
> update CompositorParent bounds from nsWindow on initialization
> 

This patch looks good to me as well, it makes sense conceptually.

Comment 30

[Geoff Brown [:gbrown]]

Passes try: https://tbpl.mozilla.org/?tree=Try&rev=02930cd79d28&noignore=1

Comment 31

[Geoff Brown [:gbrown]]

Attachment: update CompositorParent bounds from nsWindow on initialization

Cosmetic change only, as per comment 28. r=snorp (via irc)

Comment 32

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef02503c4387

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef02503c4387

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

We need this backed out on Aurora too.

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

Or rather, we need the new fix on Aurora.

Comment 37

[Geoff Brown [:gbrown]]

Really, we need to back out the original patch, rev a7a10b14ff06, from aurora, to fix bug 809005. That may cause reftest failures, so we need to also apply the new patch to aurora, rev ef02503c4387.

Comment 38

[Geoff Brown [:gbrown]]

Comment on attachment 674329 [details] [diff] [review]
update CompositorParent bounds from nsWindow on initialization

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 809005
User impact if declined: See comment 37. Fullscreen flash is broken on aurora, by an earlier patch for this bug; that patch must be backed out. Doing so will likely cause reftest failures unless this patch is then applied.
Testing completed (on m-c, etc.): on m-c since 2012-10-23
Risk to taking this patch (and alternatives if risky): Minimal risk.
String or UUID changes made by this patch: none

Comment 39

[Alex Keybl [:akeybl]]

Comment on attachment 674329 [details] [diff] [review]
update CompositorParent bounds from nsWindow on initialization

[Triage Comment]
Approving for Aurora, along with whatever backout is necessary to resolve bug 809005.

Comment 40

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3600b4bb3aac
https://hg.mozilla.org/releases/mozilla-aurora/rev/a378d29364b7