Closed Bug 1472140 Opened 2 years ago Closed 2 years ago

White page after assigning GeckoSession to a new GeckoView

Categories

(GeckoView :: General, defect, P1)

All
Android
defect

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: sebastian, Assigned: droeh)

References

Details

(Whiteboard: [geckoview:klar:p1])

Attachments

(4 files)

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.
P1 if this affects Focus+GV
Priority: -- → P1
Whiteboard: [geckoview:klar] → [geckoview:klar:p1]
Assigning to Dylan for investigation
Assignee: nobody → droeh
[geckoview:klar:p2] because Dylan says Focus has implemented a workaround.
Whiteboard: [geckoview:klar:p1] → [geckoview:klar:p2]
> [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.
See Also: → 1475586
Whiteboard: [geckoview:klar:p2] → [geckoview:klar:p1]
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.
Flags: needinfo?(s.kaspari)
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.
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)?
Flags: needinfo?(s.kaspari) → needinfo?(nchen)
Seems like it should be related, though it's hard to say what's going on with just that one line.
Flags: needinfo?(nchen)
(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.
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.
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 on attachment 8996086 [details]
Bug 1472140 - 2. Set text input / accessibility views with the session;

https://reviewboard.mozilla.org/r/260342/#review267406
Attachment #8996086 - Flags: review?(esawin) → review+
Comment on attachment 8996085 [details]
Bug 1472140 - 1. Set first paint flag when resuming and resizing composition;

https://reviewboard.mozilla.org/r/260340/#review267416
Attachment #8996085 - Flags: review?(rbarker) → review+
Pushed by nchen@mozilla.com:
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
(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?
Flags: needinfo?(nchen)
https://hg.mozilla.org/mozilla-central/rev/81459ed973a8
https://hg.mozilla.org/mozilla-central/rev/932cc42e15c1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
It would have been nice to remove the ForceIsFirstPaint IPDL message since it's no longer being used after this patch.
(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:kats@mozilla.com) 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.
Flags: needinfo?(nchen)
Remove the PCompositorBridge::ForceIsFirstPaint call that's no longer
used anywhere.
Attachment #8996360 - Flags: review?(bugmail)
Comment on attachment 8996360 [details] [diff] [review]
Bug 1472140 - 3. Remove ForceIsFirstPaint ipdl call

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

Thanks!
Attachment #8996360 - Flags: review?(bugmail) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d9fa4f595b
3. Remove ForceIsFirstPaint ipdl call; r=kats
status-firefox63=affected as per comment 20
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
Attachment #8996085 - Flags: approval-mozilla-beta?
Comment on attachment 8996085 [details]
Bug 1472140 - 1. Set first paint flag when resuming and resizing composition;

GeckoView related, Beta62+
Attachment #8996085 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.