768254 - Trying to log in using browserID within an app crashes FennecAndroid (NullPointerException at org.mozilla.gecko.GeckoApp.shouldUpdateThumbnail)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[krupa raj[:krupa]]

Fennec 16.0a1/Android

steps to reproduce:
1. Install Mozilla Marketplace app (https://marketplace-dev.allizom.org/en-US/app/mozilla-marketplace)
2. Launch the (chromeless) app.
3. Click on Login/Register link.

reproducible? yes.

observed behavior:
Trying to log in using browserID within an app crashes FennecAndroid

The crash reports are not showing up in socorro due to bug 768241

Comment 1

[Aaron Train [:aaronmt]]

This works for me on Nightly (06/25), Galaxy Nexus (Android 4.0.4). Getting that crash-report will be helpful.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Aaron Train [:aaronmt] from comment #1)
> This works for me on Nightly (06/25), Galaxy Nexus (Android 4.0.4). Getting
> that crash-report will be helpful.

Just got a crash and reproduced the behavior Krupa hit:

https://crash-stats.mozilla.com/report/index/bp-bf2c5c44-bb4e-4550-a3dd-955c22120626

Comment 3

[Jason Smith [:jsmith]]

java.lang.NullPointerException
	at org.mozilla.gecko.GeckoApp.shouldUpdateThumbnail(GeckoApp.java:790)
	at org.mozilla.gecko.GeckoApp.handleThumbnailData(GeckoApp.java:764)
	at org.mozilla.gecko.ScreenshotHandler$1.run(GeckoAppShell.java:2426)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)

Comment 5

[Jason Smith [:jsmith]]

k9o nomination - not being able to open up a persona pop-up in a web app occasionally and having a crash instead isn't a good UX experience at all. It's a very likely use case too, as Marketplace uses persona and there's other top tier apps using persona as well.

Comment 6

[Wil Clouser [:clouserw]]

Nominating to block basecamp.  In order to provide a mobile experience with the marketplace people need to be able to log in with browserid.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Wil Clouser [:clouserw] from comment #6)
> Nominating to block basecamp.  In order to provide a mobile experience with
> the marketplace people need to be able to log in with browserid.

Not a basecamp blocker, as FF for Android Web Apps is part of k9o, not basecamp. Basecamp is b2g specific.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This crash happens because mTabsPanel is null in WebApp.  This field is specific to BrowserApp, so we should move it into the subclass to avoid future bugs like this.

Unfortunately, Fennec still crashes even with this patch, when I press the BrowserID login button.  There must be some additional bug(s) when opening multiple tabs within a chromeless WebApp, but I'm having trouble debugging it because I'm not getting any stack in the Android log...

Comment 9

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 639467 [details] [diff] [review]
patch

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

I thought of moving mTabsPanel to BrowserApp. But I was told by wesj and vlad that they need to show some kind of window management for apps. :(
So, mTabsPanel should stay with GeckoApp.
Please make it "protected" so that BrowserApp can access it.
And override the shouldUpdateThumbnails() only in BrowserApp. GeckoApp returns false like in your patch.

(In short, mTabsPanel stays with GeckoApp. Other things in your patch are fine).

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 639467 [details] [diff] [review]
patch

(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> I thought of moving mTabsPanel to BrowserApp. But I was told by wesj and
> vlad that they need to show some kind of window management for apps. :(

This seems broken to me.  mTabsPanel is never used in GeckoApp or WebApp (except by the crashy code that I removed here).  It's set to a view that exists only in BrowserApp, so it is already a BrowserApp-specific property in all but name.  This patch does not change any behavior for WebApp except that it prevents a crash.  *If* we end up adding a TabsPanel to WebApp, only *then* should we move this back to the base class.

Wes and Vlad, do you need this code in GeckoApp?

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Don't believe so -- I mean, it will fix the crash, which is good, but still won't get us any kind of window mgmt for web apps on android :)

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 639467 [details] [diff] [review]
patch

Looking at the WIP patch in bug 766299, I guess we can leave this for now and let bug 766299 take care of the larger organization and UX issues -- as long as it will be finished soon so we don't leave this footgun around too long. :)

Comment 13

[Jason Smith [:jsmith]]

(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> Comment on attachment 639467 [details] [diff] [review]
> patch
> 
> Looking at the WIP patch in bug 766299, I guess we can leave this for now
> and let bug 766299 take care of the larger organization and UX issues -- as
> long as it will be finished soon so we don't leave this footgun around too
> long. :)

We have a triage for Android WebRT bugs at 10am today, so hopefully that'll answer some of the questions being brought here.

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 640265 [details] [diff] [review]
patch v2

This is a smaller patch that does not move any code out of GeckoApp.

As before, this fixes the crash in shouldUpdateThumbnail but there is still another crash that I haven't successfully debugged yet, in addition to the UX issues covered by bug 766299.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 640265 [details] [diff] [review]
patch v2

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

This looks good to me.

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/136088ebded3

Please open a new bug if there are crashes with a different signature after this patch lands.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/136088ebded3

Comment 19

[Matt Brubeck (:mbrubeck)]

Comment on attachment 640265 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 765805

User impact if declined: reproducible crash when opening a popup window from a chromeless webapp

Testing completed (on m-c, etc.): landed on m-c 7/9

Risk to taking this patch (and alternatives if risky): Very low-risk one-line mobile-only change.

String or UUID changes made by this patch: None

Comment 20

[Aaron Train [:aaronmt]]

Verified Fixed on Mozilla-Central (07/10)

Comment 21

[Jason Smith [:jsmith]]

Matt - Webapps are going to be disabled for FF 15 per bug 772162, so don't worry about uplifting this patch.

Comment 22

[krupa raj[:krupa]]

I can reproduce the issue from comment 0 with the same STR on today's Nightly. See the crash report @ https://crash-stats.mozilla.com/report/index/bp-ee273fb1-57d7-48ec-b15f-0c7002120711

Let me know if you'd prefer that I file a new bug or just reopen this one.

Comment 23

[Matt Brubeck (:mbrubeck)]

Comment on attachment 640265 [details] [diff] [review]
patch v2

(In reply to krupa raj 82[:krupa] from comment #22)
> I can reproduce the issue from comment 0 with the same STR on today's
> Nightly. See the crash report @
> https://crash-stats.mozilla.com/report/index/bp-ee273fb1-57d7-48ec-b15f-
> 0c7002120711
> 
> Let me know if you'd prefer that I file a new bug or just reopen this one.

It looks like that is a different crash, which has already been filed as bug 770263 and may be fixed in today's nightly build.