Closed Bug 1320587 Opened 3 years ago Closed 3 years ago

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

Categories

(GeckoView :: General, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: nalexander, Unassigned)

References

Details

Attachments

(1 file)

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?
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.
Flags: needinfo?(nchen)
I would like to see all of this code (the background setting stuff that uses first-paint) just die.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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?
Flags: needinfo?(snorp)
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?
Flags: needinfo?(snorp) → needinfo?(rbarker)
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.
Flags: needinfo?(nchen)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(rbarker)
(In reply to Randall Barker [:rbarker] from comment #6)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Attachment #8815614 - Flags: review?(nchen)
Comment on attachment 8815614 [details]
Bug 1320587 - Force first paint when (re-)attaching Gecko to Java.

https://reviewboard.mozilla.org/r/96492/#review96866
Attachment #8815614 - Flags: review?(nchen) → review+
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/754947cc763c
Force first paint when (re-)attaching Gecko to Java. r=jchen
https://hg.mozilla.org/mozilla-central/rev/754947cc763c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1321638
No longer depends on: 1321638
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.