1320587 - [geckoview] Compositor doesn't set first paint correctly for non-GeckoApp based GeckoView consuming Activities

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

I can't fully explain what's happening here, but here's a shot:

If I embed a GeckoView into a non-GeckoApp based Activity, the initial paint succeeds.  However, on rotation, I get a white GeckoView.  I've traced the code, and I think *almost* everything is working: the resize and rotation events, etc, are being handled, the GeckoView is being re-created (with a new Java instance) by the Android framework, and the compositor is re-attached to the new GeckoView instance, but still, I get a blank white paint.

I've traced this to the following logic around resetting: http://searchfox.org/mozilla-central/source/widget/android/nsWindow.cpp#1076.  This was introduced in Bug 1227706, and appears to work for BrowserApp (which painfully maintains a single GeckoView throughout the App lifecycle) and for CustomTabsActivity (which plugins into the browser.js tab management code, which I think invalidates the content document in a way that works around the bug).

What I see in my test application (which I will try to push somewhere shortly) is that AttachToJava is called only once per GeckoView.  This makes sense, since each GeckoView allocates a new Compositor (see http://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java#126) and therefore mLayerClient is always null, even when we need to force the invalidation.

When I force resetting to be true locally, everything works.  I conclude that we should either always be resetting; compare aClient to mLayerClient, accepting null mLayerClient as needing resetting; or figure out the "higher level interpretation" -- perhaps we're supposed to be reusing Compositor instances across GeckoView instances?

Comment 1

[Nick Alexander :nalexander [he/him]]

jchen: can you comment on the correct approach here, and hopefully just fix it in the way you see fit?  I have a provisional patch and will add tests for this when I can.

Comment 2

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

I would like to see all of this code (the background setting stuff that uses first-paint) just die.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> I would like to see all of this code (the background setting stuff that uses
> first-paint) just die.

I would be happy to see this as well, since it's an obviously delicate and race-prone part of the code.  Can you sketch what needs to happen?

Comment 4

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

I think the reason we have this is so we can set a specific background color before a page has rendered. IIRC, this is done to avoid weirdness when showing the LayerView when coming from the awesome screen, etc. There are probably better ways to do this.

Randall messed with this a little bit when removing JPZ, maybe he has insight?

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

I think we might just be running into a race where we reattach the compositor after resuming it, so the compositor doesn't know it has to repaint. FWIW, the straight-forward fix would be to always reset.

Comment 6

[Randall Barker [:rbarker]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> I think the reason we have this is so we can set a specific background color
> before a page has rendered. IIRC, this is done to avoid weirdness when
> showing the LayerView when coming from the awesome screen, etc. There are
> probably better ways to do this.
> 
> Randall messed with this a little bit when removing JPZ, maybe he has
> insight?

It isn't about setting the background color, The issue is we cover the surface with an opaque widget until the compositor has started painting, otherwise we get a flash of unpainted surface (which is white). Once the compositor has signaled that is has painted, we hide the covering widget. If we never get the call to signal the first paint, we never remove the covering widget. It looks like nothing is being rendered but in fact is just hidden behind the covering widget.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Randall Barker [:rbarker] from comment #6)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> > I think the reason we have this is so we can set a specific background color
> > before a page has rendered. IIRC, this is done to avoid weirdness when
> > showing the LayerView when coming from the awesome screen, etc. There are
> > probably better ways to do this.
> > 
> > Randall messed with this a little bit when removing JPZ, maybe he has
> > insight?
> 
> It isn't about setting the background color, The issue is we cover the
> surface with an opaque widget until the compositor has started painting,
> otherwise we get a flash of unpainted surface (which is white). Once the
> compositor has signaled that is has painted, we hide the covering widget. If
> we never get the call to signal the first paint, we never remove the
> covering widget. It looks like nothing is being rendered but in fact is just
> hidden behind the covering widget.

Thank you, this is extremely useful to know.  I will verify that I am seeing this, which should be easy on the Java side.

I think I'll keep this ticket for just always resetting, and let you fine folks follow up with a "tear it out/improve it all" ticket if you deem fit.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1320587 - Force first paint when (re-)attaching Gecko to Java.

Review commit: https://reviewboard.mozilla.org/r/96492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96492/

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8815614 [details]
Bug 1320587 - Force first paint when (re-)attaching Gecko to Java.

https://reviewboard.mozilla.org/r/96492/#review96866

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/754947cc763c
Force first paint when (re-)attaching Gecko to Java. r=jchen

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/754947cc763c