1472140 - White page after assigning GeckoSession to a new GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

This is kind of a follow-up from bug 1470883.

I have a working browser prototype that switches tabs by assigning GeckoSession objects to GeckoViews. I'm using GeckoView 63.0.20180627100018.

To avoid running into the problems mentioned in bug 1470883 I release the session in onDetachedFromWindow() so that GeckoView won't close it:

>    internal var gec = objekoView : GeckoView(context) {
>        override fun onDetachedFromWindow() {
>            releaseSession()
> 
>            super.onDetachedFromWindow()
>        }
>    }

Right now I create a new Fragment (and GeckoView) whenever I display a new/selected tab and assign the matching GeckoSession.

This only works once for every GeckoSession. As soon as I assign a GeckoSession to a GeckoView for the second time it only renders a white page. New GeckoSession objects display fine. We are not seeing this in issue because currently we always save/restore into a new GeckoSession... but that's something we do not want to continue doing.

Comment 1

[Chris Peterson [:cpeterson]]

P1 if this affects Focus+GV

Comment 2

[Chris Peterson [:cpeterson]]

Assigning to Dylan for investigation

Comment 3

[Chris Peterson [:cpeterson]]

[geckoview:klar:p2] because Dylan says Focus has implemented a workaround.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

> [geckoview:klar:p2] because Dylan says Focus has implemented a workaround.

Re-using GeckoSession was our workaround for avoiding to save state that hangs infrequently. Either one of those requires a fix or we cannot implement tab switching with GeckoView.

Comment 5

[Dylan Roeh (:droeh) (he/him)]

Sebastian, is there a branch of Focus I can use to test this? I've been trying to reproduce in GVE and everything I try works fine.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

I can try setting a branch up with Focus tomorrow. If you want something today you can try the sample browser in the components repository:
https://github.com/mozilla-mobile/android-components

Import the repo into Android Studio and launch "samples-browser" (Make sure to use a "nightly" build variant). Then in the browser switch between tabs. As soon as you select the same tab twice it will render a white page.

Comment 7

[Dylan Roeh (:droeh) (he/him)]

Thanks, the sample browser was good enough to get me on the right track. We can actually reuse sessions across multiple views if those views are in the layout -- it appears to be reusing sessions with programmatically created views that is the issue, as far as I can tell.

Jim, I'm seeing a logcat message:
E SurfaceFlinger: Failed to find layer (SurfaceView - org.mozilla.geckoview_example/org.mozilla.geckoview_example.GeckoViewActivity#0) in layer parent (no-parent).

when this occurs; do you have any idea what's going on here (or at least if it's likely to be related to the white screen issue)?

Comment 8

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

Seems like it should be related, though it's hard to say what's going on with just that one line.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #8)
> Seems like it should be related, though it's hard to say what's going on
> with just that one line.

Unfortunately there wasn't much else that seemed relevant iirc. I'm wrapping up something else at the moment, but I'll post a simple testing patch and full log later.

Comment 10

[Dylan Roeh (:droeh) (he/him)]

Attachment: Minimal test case

Here's a minimal test case in GVE. I'm beginning to think snorp is right about the SurfaceFlinger message being a red herring, though: we get the SurfaceFlinger message on calling replaceView(), but only suffer the white screen problem if we try setting the session on the new view to the existing session -- if we leave it with its own new session, we properly load and display pages as expected.

Comment 11

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

Attachment: Bug 1472140 - 1. Set first paint flag when resuming and resizing composition;

When resuming composition in a new GeckoView, we wait for the first
paint signal in order to uncover the SurfaceView. This patch makes sure
that we always send the first paint signal.

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

Comment 12

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

Attachment: Bug 1472140 - 2. Set text input / accessibility views with the session;

Instead of changing the text input / accessibility views during window
attachment / detachment, we should change them during session attachment
/ detachment because those views are associated with the session.

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

Comment 13

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

Looked into this with Dylan's test case. Seems like we're not firing the first paint callbacks properly when switching surfaces. Fix is really simple so I went ahead and wrote a couple patches.

Comment 14

[Eugen Sawin [:esawin]]

Comment on attachment 8996086 [details]
Bug 1472140 - 2. Set text input / accessibility views with the session;

https://reviewboard.mozilla.org/r/260342/#review267406

Comment 15

[Randall Barker [:rbarker]]

Comment on attachment 8996085 [details]
Bug 1472140 - 1. Set first paint flag when resuming and resizing composition;

https://reviewboard.mozilla.org/r/260340/#review267416

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81459ed973a8
1. Set first paint flag when resuming and resizing composition; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/932cc42e15c1
2. Set text input / accessibility views with the session; r=esawin

Comment 17

[Chris Peterson [:cpeterson]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> > [geckoview:klar:p2] because Dylan says Focus has implemented a workaround.
> 
> Re-using GeckoSession was our workaround for avoiding to save state that
> hangs infrequently. Either one of those requires a fix or we cannot
> implement tab switching with GeckoView.

Jim, do we need to uplift your fix to 62 Beta for the Focus 7.0 beta test? Does Focus need to make any code changes to its use of GeckoSession after your fix?

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/81459ed973a8
https://hg.mozilla.org/mozilla-central/rev/932cc42e15c1

Comment 19

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

It would have been nice to remove the ForceIsFirstPaint IPDL message since it's no longer being used after this patch.

Comment 20

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

(In reply to Chris Peterson [:cpeterson] from comment #17)
> (In reply to Sebastian Kaspari (:sebastian) from comment #4)
> > > [geckoview:klar:p2] because Dylan says Focus has implemented a workaround.
> > 
> > Re-using GeckoSession was our workaround for avoiding to save state that
> > hangs infrequently. Either one of those requires a fix or we cannot
> > implement tab switching with GeckoView.
> 
> Jim, do we need to uplift your fix to 62 Beta for the Focus 7.0 beta test?
> Does Focus need to make any code changes to its use of GeckoSession after
> your fix?

We should uplift. With this fix I think Focus can start doing session-switching instead of session-restoring when switching tabs. So yes there needs to be Focus code changes as well.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> It would have been nice to remove the ForceIsFirstPaint IPDL message since
> it's no longer being used after this patch.

Good point! I can push a followup.

Comment 21

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

Attachment: Bug 1472140 - 3. Remove ForceIsFirstPaint ipdl call

Remove the PCompositorBridge::ForceIsFirstPaint call that's no longer
used anywhere.

Comment 22

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

Comment on attachment 8996360 [details] [diff] [review]
Bug 1472140 - 3. Remove ForceIsFirstPaint ipdl call

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

Thanks!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d9fa4f595b
3. Remove ForceIsFirstPaint ipdl call; r=kats

Comment 24

[Chris Peterson [:cpeterson]]

status-firefox63=affected as per comment 20

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/78d9fa4f595b

Comment 26

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

Comment on attachment 8996085 [details]
Bug 1472140 - 1. Set first paint flag when resuming and resizing composition;

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: White screen when switching sessions in GeckoView app.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: GeckoView-only fix that should have minimal impact on Fennec
[String changes made/needed]: None

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8996085 [details]
Bug 1472140 - 1. Set first paint flag when resuming and resizing composition;

GeckoView related, Beta62+

Comment 28

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b7c86a171b24
https://hg.mozilla.org/releases/mozilla-beta/rev/d0ffdf9419b5
https://hg.mozilla.org/releases/mozilla-beta/rev/43ad096a472b