749624 - Sites don't load, logcat says BrowserApp.selectedTab is null

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Johnathan Nightingale [:johnath]]

Attachment: logcat

I occassionally get into a state where pages won't load (which, I realize, sounds like bug 705558 RESO WFM). The throbber spins, the awesomescreen is responsive, but no content ever loads. logcat says:

JavaScript Error: "TypeError: BrowserApp.selectedTab is null" {file: "chrome://browser/content/browser.js" line: 4619}

Showed Margaret during work week, she agreed it was worth filing, though it might be dup'd by work sriram was doing.

More logcat attached.

Nom'ng because failure-to-load is a big problem, though I don't have any STR so it might be tough to QA.

Comment 1

[:Margaret Leibovic]

This looks like code introduced by bug 736311.

Is it possible there's a legitimate state where we're calling this observer when BrowserApp.selectedTab is null? It looks like throwing a null check in there would fix the problem, but we should make sure this isn't part of a bigger issue.

Comment 2

[Justin Lebar (not reading bugmail)]

> This looks like code introduced by bug 736311.

How, exactly?  I don't see us modifying the value of selectedTab in that patch.

https://hg.mozilla.org/mozilla-central/rev/0bc32fccce4c

Comment 3

[:Margaret Leibovic]

(In reply to Justin Lebar [:jlebar] from comment #2)
> > This looks like code introduced by bug 736311.
> 
> How, exactly?  I don't see us modifying the value of selectedTab in that
> patch.
> 
> https://hg.mozilla.org/mozilla-central/rev/0bc32fccce4c

The problem isn't that we're modifying it, the problem is that we're assuming it's not null in this observer:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4749

If it is null, we get an error and the JS side of the app dies.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Please see bug 749149 for my STR if needed.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Johnathan Nightingale [:johnath] from comment #0)
> Showed Margaret during work week, she agreed it was worth filing, though it
> might be dup'd by work sriram was doing.

Definitely worth filing - this could drive users back to stock.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

My logcat here: https://bug749149.bugzilla.mozilla.org/attachment.cgi?id=619087

Comment 8

[:Margaret Leibovic]

Attachment: patch

Looking more closely at the log, this happened right after startup. We fire the application-foreground notification on ACTIVITY_START [1], so it's totally possible to get into a case where we observe that before setting up a selected tab. Therefore, this null check seems like a safe way to go.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#431

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

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

With all the "BrowserApp.selectedTab" usage can you create a local variable?

let browser = BrowserApp.selectedTab;
if (browser && browser.getActive() != isForeground)
  browser.setActive(isForeground);

Comment 10

[:Margaret Leibovic]

I used |tab| instead of |browser|, since that's a better variable name.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8352ac30ddcd

Comment 11

[:Margaret Leibovic]

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

[Approval Request Comment]
Regression caused by (bug #): bug 736311
User impact if declined: you can get into a state where the gecko side of the app dies
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low risk (just add a null check)
String changes made by this patch: n/a

Comment 12

[Aaron Train [:aaronmt]]

Running into this with a debug build this morning

Comment 13

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #12)
> Running into this with a debug build this morning

What tree? This already landed on inbound.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/8352ac30ddcd

Comment 15

[JP Rosevear [:jpr]]

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

Mobile only.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e32c74580f6