1440592 - Crash in java.lang.IllegalStateException: Unexpected new session at org.mozilla.gecko.customtabs.CustomTabsActivity.onNewSession(CustomTabsActivity.java)

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-b4d8b827-a05f-418d-830f-2e5d70180223.
=============================================================

java.lang.IllegalStateException: Unexpected new session
	at org.mozilla.gecko.customtabs.CustomTabsActivity.onNewSession(CustomTabsActivity.java:656)
	at org.mozilla.geckoview.GeckoSession$2.handleMessage(GeckoSession.java:150)
	at org.mozilla.geckoview.GeckoSession$2.handleMessage(GeckoSession.java:127)
	at org.mozilla.geckoview.GeckoSessionHandler.handleMessage(GeckoSessionHandler.java:91)
	at org.mozilla.gecko.EventDispatcher$2.run(EventDispatcher.java:341)
	at android.os.Handler.handleCallback(Handler.java:751)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:154)
	at android.app.ActivityThread.main(ActivityThread.java:6316)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:872)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:762)

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

There are 16 crashes (from 12 installations) in nightly 60 with buildid 20180223001836. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1432485.

[1] https://hg.mozilla.org/mozilla-central/rev?node=d39e77de7c54

Comment 1

[Calixte Denizet (:calixte)]

The signature "java.lang.IllegalStateException: Unexpected new session at org.mozilla.gecko.customtabs.CustomTabsActivity.onNewSession(CustomTabsActivity.java)" is ranked #1 in nightly top-crashers for FennecAndroid.

Comment 2

[Marcia Knous [:marcia]]

530 crashes/369 since this was initially filed.

Comment 3

[Jamie Nicol [:jnicol]]

I can reproduce this. I opened a link to a facebook video from the reddit app. Then when I tap play it crashes.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1440592-1.0-Use-correct-where-constant-for-new-w.patch

In bug 1396065 we've used the wrong constant for new window and with the introduction of onNewSession we run into unexpected behavior because of it.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8954365 [details] [diff] [review]
0001-Bug-1440592-1.0-Use-correct-where-constant-for-new-w.patch

uh, those flags are confusing.

Comment 6

[Sorina Florean [:sflorean]]

I was able to reproduce this crash 5/5 times on HTC 10(Android 8.1.0), Samsung Galaxy S8(Android 7.0) and Nexus 5 (Android 6.0.1) following the steps:
1. Open a page using Custom Tab;
2. Long tap on a word and from selection menu choose Google search.
Actual results: Nightly crashes and the search query is not opened.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcadcf486b78
[1.0] Use correct where constant for new window. r=smaug

Comment 8

[Cristina Coroiu [:ccoroiu]]

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

Comment 9

[Calixte Denizet (:calixte)]

There still are 299 crashes in nightly 60 with buildid >= 20180301223345 (see [1]) with signature "java.lang.IllegalStateException: Unexpected new session at org.mozilla.gecko.customtabs.CustomTabsActivity.onNewSession(CustomTabsActivity.java)".

[1] http://bit.ly/2oLimb4

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1440592-2.0-Enable-URI-loading-delegation-in-ope.patch

We need to handle openURI calls according to our general scheme (1. delegate to app, 2. create new session if required, 3. load the URI).

Comment 11

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1440592-2.0-Enable-URI-loading-delegation-in-ope.patch

Removed unnecessary module import.

Comment 12

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

Comment on attachment 8957561 [details] [diff] [review]
0001-Bug-1440592-2.0-Enable-URI-loading-delegation-in-ope.patch

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

::: mobile/android/modules/geckoview/LoadURIDelegate.jsm
@@ +11,5 @@
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  Services: "resource://gre/modules/Services.jsm",
> +});
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "ParentalControls",

We don't use this

@@ +19,5 @@
> +  // Delegate URI loading to the app.
> +  // Return whether the loading has been handled.
> +  load: function(aEventDispatcher, aUri, aWhere, aFlags, aTriggeringPrincipal) {
> +    const message = {
> +      type: "GeckoView:OnLoadUri",

The public thing is onLoadRequest() now, maybe we should rename these messages too while we're at it.

Comment 13

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

Comment on attachment 8957563 [details] [diff] [review]
0002-Bug-1440592-2.0-Enable-URI-loading-delegation-in-ope.patch

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

I guess I reviewed the wrong one earlier?

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1440592-3.0-Rename-GeckoView-event-onLoadURI-to-.patch

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80169b025661
[2.0] Enable URI-loading delegation in openURI. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0371f3d5f7
[3.0] Rename GeckoView event onLoadURI to onLoadRequest. r=snorp

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9d1c92da9b
[4.0] Fix linter errors on CLOSED TREE. r=me

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5d6034a9ca
[5.0] Fix merge error with bug 1422084 on CLOSED TREE. r=me

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/80169b025661
https://hg.mozilla.org/mozilla-central/rev/7f0371f3d5f7
https://hg.mozilla.org/mozilla-central/rev/2c9d1c92da9b
https://hg.mozilla.org/mozilla-central/rev/4f5d6034a9ca

Comment 19

[Julien Cristau [:jcristau]]

This is still around in 60.0b3 (and 61.0a1):

https://crash-stats.mozilla.com/signature/?product=FennecAndroid&version=60.0b&signature=java.lang.IllegalStateException%3A Unexpected new session at org.mozilla.gecko.customtabs.CustomTabsActivity.onNewSession(CustomTabsActivity.java)

Comment 20

[Eugen Sawin [:esawin]]

The remaining crashes might be related to the startup race, let's see if bug 1446423 helps.

Comment 21

[Julien Cristau [:jcristau]]

By far the top crasher in 60.0b4

Comment 22

[Julien Cristau [:jcristau]]

Eugen, are you still looking at this?

Comment 23

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1440592-4.0-Allow-load-delegation-when-opening-w.patch

There are paths for window creations (nsWindowWatcher::OpenWindowInternal) which do not call into the docshell before creating a new window. Currently, this would result in createContentWindow and therefore onNewSession calls without prior option to deflect the creation via onLoadRequest.

We need to either allow onNewSession to return the current session, which would require adjustments in Gecko, or we guarantee that onLoadRequest is always called prior to onNewSession to allow the app to handle that appropriately.

This patch implements the latter solution by partly restoring the old (pre-onNewSession) handling for content window creation. We need to delegate the load to the app before requesting the new session. On success, we need to abort the handling in Gecko by throwing NS_ERROR_ABORT to prevent further attempts of window creation and attaching. This will bring back the old JavaScript exception log.

Comment 24

[Eugen Sawin [:esawin]]

New STR would include a page with a target=_blank link inside an iframe.

Comment 25

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

Comment on attachment 8962717 [details] [diff] [review]
0004-Bug-1440592-4.0-Allow-load-delegation-when-opening-w.patch

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

I think I would rather just accept that onLoadRequest() isn't called sometimes when onNewSession() is called, and just stop throwing here in CustomTabsActivity, etc. Let's get a bug on file for the underlying issue, though.

onNewSession() and onLoadRequest() are pretty much equivalent for the "new window" case, so maybe it's ok to specify that onLoadRequest() is only called for navigations within the session?

Comment 26

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

I think we have a few options here:

1) Change the behavior of onLoadRequest() such that it is only fired for loads in the same session. We would remove the 'target' argument entirely, since it's always assumed to be the current session. onNewSession() would be how you prevent or otherwise handle loads that need a new session. I think this would simply need a patch in the docshell to check some flags before calling onLoadRequest().

2) Plug the current hole by calling onLoadRequest() in the cases we aren't handling it now. It should be possible to get the originating docshell and call onLoadRequest() in nsWindowWatcher::OpenWindowInternal(), for instance.

3) Overhaul the whole API around these two delegates. In bug 1437701 I'll be adding yet another "handle loading this URI" type of delegate method, and they all seem very similar. It seems like we should be able to do better.

Comment 27

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1440592-4.1-Allow-load-delegation-when-opening-w.patch

(In reply to James Willcox (:snorp) ([e-mail]) from comment #26)
> I think we have a few options here:
> 
> 1) Change the behavior of onLoadRequest() such that it is only fired for
> loads in the same session. We would remove the 'target' argument entirely,
> since it's always assumed to be the current session. onNewSession() would be
> how you prevent or otherwise handle loads that need a new session. I think
> this would simply need a patch in the docshell to check some flags before
> calling onLoadRequest().

With the current constraints of onNewSession, it would make it difficult for the app to appropriately handle some cases and signal correct behavior back to Gecko.
I think a single delegate callback for all page load-related overrides is preferable and easier to communicate.

> 2) Plug the current hole by calling onLoadRequest() in the cases we aren't
> handling it now. It should be possible to get the originating docshell and
> call onLoadRequest() in nsWindowWatcher::OpenWindowInternal(), for instance.

Handling this in GVNavigation will ensure we enclose all code paths and signal NS_ERROR_ABORT to stop Gecko from handling it. This would also be more robust against changes on Gecko's side.
 
> 3) Overhaul the whole API around these two delegates. In bug 1437701 I'll be
> adding yet another "handle loading this URI" type of delegate method, and
> they all seem very similar. It seems like we should be able to do better.

I agree that the API could be better, we can follow up on that.

Comment 28

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

Comment on attachment 8962785 [details] [diff] [review]
0004-Bug-1440592-4.1-Allow-load-delegation-when-opening-w.patch

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

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@@ +184,5 @@
>      if (browser) {
>        browser.setAttribute("nextTabParentId", aNextTabParentId);
>      }
>  
>      return browser;

I wonder, do we want to set NS_ERROR_ABORT here if browser is null?

@@ +200,3 @@
>      }
>  
> +    let browser = this.browser;

const, event though it was wrong before

Comment 29

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1440592-4.2-Allow-load-delegation-when-opening-w.patch

(In reply to James Willcox (:snorp) ([e-mail]) from comment #28)
> Comment on attachment 8962785 [details] [diff] [review]
> 0004-Bug-1440592-4.1-Allow-load-delegation-when-opening-w.patch
> 
> Review of attachment 8962785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm
> @@ +184,5 @@
> >      if (browser) {
> >        browser.setAttribute("nextTabParentId", aNextTabParentId);
> >      }
> >  
> >      return browser;
> 
> I wonder, do we want to set NS_ERROR_ABORT here if browser is null?

Good point, I think we should set it and the same holds for createContentWindow.
Returning a null window with NS_OK triggers Gecko's window creator path, which will fail for GV.

> @@ +200,3 @@
> >      }
> >  
> > +    let browser = this.browser;
> 
> const, event though it was wrong before

browser gets reassigned.

Comment 30

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

Comment on attachment 8962820 [details] [diff] [review]
0004-Bug-1440592-4.2-Allow-load-delegation-when-opening-w.patch

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

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@@ +164,5 @@
> +    if (browser) {
> +      return browser.contentWindow;
> +    }
> +
> +    Components.returnCode = Cr.NS_ERROR_ABORT;

So we'll end up returning 'undefined' here which seems a bit weird. Nobody should be using the return value, though, if the call failed. Maybe invert the condition above if you feel like it.

@@ +180,5 @@
>  
> +    if (LoadURIDelegate.load(this.eventDispatcher, aUri, aWhere, aFlags, null)) {
> +      // The app has handled the load, abort open-window handling.
> +      Components.returnCode = Cr.NS_ERROR_ABORT;
> +      return;

Same as above, maybe we should return null. Just seems wrong to sometimes return a value and other times not.

Comment 31

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1440592-4.3-Allow-load-delegation-when-opening-w.patch

Addressed comments.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15eed3a9a8e3
[4.3] Allow load delegation when opening windows before requesting a new session. r=snorp

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/15eed3a9a8e3

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance so we can get it in for Monday's b9 build.

Comment 35

[Eugen Sawin [:esawin]]

Comment on attachment 8962867 [details] [diff] [review]
0004-Bug-1440592-4.3-Allow-load-delegation-when-opening-w.patch

Looks like this is the only patch that didn't make it to 60 and bug 1446423 didn't seem to move crash stats at all.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1432485.
[User impact if declined]: Crash on page load.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Verified locally, crash stats went down to 0 crashes.
[Needs manual test from QE? If yes, steps to reproduce]: Maybe? STR: Open page with embedded YouTube video, click on the title of the video inside the video frame. 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Somewhat.
[Why is the change risky/not risky?]: It changes behavior for some cases of page loading, but it only affects custom tabs and web apps.
[String changes made/needed]: None.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962867 [details] [diff] [review]
0004-Bug-1440592-4.3-Allow-load-delegation-when-opening-w.patch

Fennec topcrash fix. Approved for 60.0b9.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d244d295cd2b