1435500 - Crash in java.lang.IllegalStateException: Current session is open at org.mozilla.gecko.GeckoView.setSession(GeckoView.java)

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-e2947432-16b7-434b-820f-7fd7b0180131.
=============================================================

Java Stack Trace 	

java.lang.IllegalStateException: Current session is open
	at org.mozilla.gecko.GeckoView.setSession(GeckoView.java:194)
	at org.mozilla.gecko.GeckoApp.onCreate(GeckoApp.java:1037)
	at org.mozilla.gecko.BrowserApp.onCreate(BrowserApp.java:653)
	at android.app.Activity.performCreate(Activity.java:6999)
	at android.app.Activity.performCreate(Activity.java:6990)
	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1214)
	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2731)
	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2856)
	at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4699)
	at android.app.ActivityThread.-wrap18(Unknown Source:0)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1595)
	at android.os.Handler.dispatchMessage(Handler.java:106)
	at android.os.Looper.loop(Looper.java:164)
	at android.app.ActivityThread.main(ActivityThread.java:6494)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:440)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)
	at de.robv.android.xposed.XposedBridge.main(XposedBridge.java:108)

=============================================================

this crash signature starting on fennec 58 seems to be a regression from bug 1413698

Comment 1

[Mike Taylor [:miketaylr]]

Snorp, is this important enough to try to get into 59?

Comment 2

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

Should be an easy fix.

Dylan I think setSession() should be fine with a session that is already open. Can you write that patch and uplift to 59?

Comment 3

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

Attachment: Allow setting a new session on a GeckoView

I think it suffices here just to remove the existing check. I tested by modifying custom tabs to create a second GeckoSession and set the GeckoView's session to the new one -- things work as expected.

Comment 4

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

Comment on attachment 8949415 [details] [diff] [review]
Allow setting a new session on a GeckoView

We will leak an open session here meaning we will leak an open nsWindow, which is bad. We should investigate why this is happening rather than just ignore the leak.

Comment 6

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

Attachment: Call GeckoSession.closeWindow() in GeckoApp.onDestroy()

I can't reproduce this, but it seems like it may be similar to bug 1434126, where we were not closing the GeckoSession when PWAs/Custom Tabs were killed. If so, this should fix it.

Comment 7

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

Comment on attachment 8954448 [details] [diff] [review]
Call GeckoSession.closeWindow() in GeckoApp.onDestroy()

I'm not sure we want to always close the session. Why not do this in onCreate before `mLayerView.setSession`?

Comment 8

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

Attachment: Close the existing GeckoSession (if any) in GeckoApp.onCreate()

Fair enough, is there something specific the session might need to do after onDestroy()?

Comment 9

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

Comment on attachment 8954842 [details] [diff] [review]
Close the existing GeckoSession (if any) in GeckoApp.onCreate()

Please add a comment.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9f4d34d9451
Close the existing GeckoSession if it exists during GeckoApp.onCreate()  r=jchen

Comment 11

[Cristina Coroiu [:ccoroiu]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request release approval on this ASAP if you think it should be uplifted to 59 still.

Comment 13

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

Comment on attachment 8954842 [details] [diff] [review]
Close the existing GeckoSession (if any) in GeckoApp.onCreate()

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Potential start-up crash
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[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?]: No
[Why is the change risky/not risky?]: Behavior should only change if we were in a state that would cause a crash.
[String changes made/needed]: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8954842 [details] [diff] [review]
Close the existing GeckoSession (if any) in GeckoApp.onCreate()

Fixes a startup crash by adding code to check for being in a known-bad state. Approved for Fennec 59b15.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/548813e997b9 (FIREFOX_59b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/91d923c2c0c2