Restoring all tabs from previous session fails because of "org.json.JSONException: No value for isPrivate"

VERIFIED FIXED in Firefox 47

Status

()

Firefox for Android
General
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: strugee, Assigned: JanH)

Tracking

unspecified
Firefox 49
All
Android
Points:
---

Firefox Tracking Flags

(firefox46 unaffected, firefox47 verified, firefox48 verified, firefox49 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8754519 [details]
adb logcat

Recently I had Firefox crash on my Android device. Sadface (although I can't complain because I really asked for it by running a Nightly).

After it restarted, I was shown the Awesomescreen "Recent Tabs" view. Since I'm rather attached to my tabs, I scrolled to the bottom of the "tabs from last time" list and hit "Open all". This opened the first six tabs, and didn't do anything about the rest of the 100-200 tabs I have open. Additionally, tapping on the individual tabs in the list doesn't do anything unless I tap on one which was opened by "Open all".

`adb logcat` reveals the following (I'll attach a full log too):

05-19 12:58:58.191  9759 10002 D GeckoTabs: handleMessage: Tab:Added
05-19 12:58:58.200  9759 10002 W GeckoTabs: handleMessage threw for Tab:Added
05-19 12:58:58.200  9759 10002 W GeckoTabs: org.json.JSONException: No value for isPrivate
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.json.JSONObject.get(JSONObject.java:389)
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.json.JSONObject.getBoolean(JSONObject.java:410)
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.mozilla.gecko.Tabs.handleMessage(Tabs.java:461)
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:325)
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:248)
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:1911)
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
05-19 12:58:58.200  9759 10002 W GeckoTabs: 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:509)
05-19 12:58:58.204  9759 10002 D GeckoTabs: handleMessage: Content:StateChange
05-19 12:58:58.210  9759  9759 D GeckoNetworkManager: Incoming event enableNotifications for state OnNoListeners -> OnWithListeners
05-19 12:58:58.211  9759  9759 D GeckoNetworkManager: New network state: UP, WIFI, WIFI
05-19 12:58:58.213  9759  9759 D GeckoNetworkManager: Incoming event receivedUpdate for state OnWithListeners -> OnWithListeners
05-19 12:58:58.214  9759  9759 D GeckoNetworkManager: New network state: UP, WIFI, WIFI
05-19 12:58:58.216  9759 10002 D GeckoTabs: handleMessage: Content:StateChange
05-19 12:58:58.227  9759 10002 D GeckoTabs: handleMessage: Content:StateChange

It's worth noting that from the `GeckoConsole` messages I get further down, it seems that the tabs _are_ actually loading - they're just not showing in the UI. Also, the attached log is me sitting on the homescreen for a bit, tapping the Nightly icon, scrolling (way) down to the "Open all" button, hitting it, and sitting in Firefox for a little bit.

I'm on Firefox Nightly, built 2016/05/18, running on CyanogenMod 13.0-20160516-NIGHTLY-angler (which is based on Android 6.0.1). I own a Nexus 6P.
(Assignee)

Comment 1

2 years ago
I have a suspicion what might have gone wrong - over in bug 1229259, I've changed tab initialisation to always include a basic amount of session store data. Previously, there was a small window of time between tab creation and DOMTitleChanged where a tab wouldn't have any session store data and therefore wouldn't get saved to disc. Now that tabs are always initialised with that basic amount of session store data, it seems possible that they might get saved to disk in that state if a session save is triggered before the full tab data is collected by the session store.

This doesn't seem to be a problem for the session restore on startup, because the Java session parser there treats isPrivate as optional [1], however the Recent Tabs panel works in a slightly different manner.
Any click on a tab (or the Open All button) ends up here [2], which means that if isPrivate is not present in the session store data, we end up calling BrowserApp.addTab() with params.isPrivate explicitely set to "undefined", which coincidentally defeats browser.js's default value logic [3].
So in the end, Java receives a Tab:Added message with isPrivate undefined, which means things will fall apart when we want to access that value [4] while trying to actually create a new tab on the Java UI side.

[1] https://dxr.mozilla.org/mozilla-central/rev/f1f2644d34440fe38c098e323bf570e09197e51a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1670
[2] https://dxr.mozilla.org/mozilla-central/rev/f1f2644d34440fe38c098e323bf570e09197e51a/mobile/android/components/SessionStore.js#1089
[3] https://dxr.mozilla.org/mozilla-central/rev/f1f2644d34440fe38c098e323bf570e09197e51a/mobile/android/chrome/content/browser.js#3431
[4] https://dxr.mozilla.org/mozilla-central/rev/f1f2644d34440fe38c098e323bf570e09197e51a/mobile/android/base/java/org/mozilla/gecko/Tabs.java#464
Assignee: nobody → jh+bugzilla
Blocks: 1229259
Status: UNCONFIRMED → NEW
status-firefox46: --- → unaffected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
(Assignee)

Comment 2

2 years ago
STR:
1. Turn session store logging on (browser.sessionstore.debug_logging to true and restart).
2. Turn tab restoring off ("never").
3. Watch logcat for GeckoSessionStore and launch Firefox.
4. Open any page with a link to a large page which will take a while to load (e.g. for me https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile+path%3Abrowser.js&redirect=false)
5. Open the link (e.g. the link to browser.js) in a new tab and then immediately hit the home button. You want to time this to happen after the session store has processed the TabOpen event, so the new tab with its basic session store data gets written to storage on backgrounding.
6. Once the session store has finished writing (watch logcat for _writeFile() _write() returned), kill Firefox. Either use the stop button in Android Studio, or keep a terminal with |adb shell am force-stop org.mozilla.fennec| ready. You must do this before the session store starts writing the updated full tab data to disk after receiving the DOMTitleChanged event for the new tab (that's why you should pick a page which is slow to load).
7. Relaunch Firefox and try restoring the new tab from the "Tabs from last time", which should give you the symptoms described by Alex (Gecko starts loading the tab, however it doesn't appear in the UI, instead you get errors in the logcat from GeckoTabs).
(Assignee)

Comment 3

2 years ago
Created attachment 8755138 [details]
MozReview Request: Bug 1274390 - Include isPrivate and desktopMode in the session store data used for initialising new tabs. r=sebastian

Restoring a tab from the Recent Tabs panel, which goes via the session store's _restoreTabs() function and ultimately via BrowserApp.addTab() and a Tab:Added message back to the Java UI requires the value for isPrivate to be present in the session store data for the respective tab - if it isn't, we end up sending isPrivate as "undefined", which breaks the process of adding the new tab in our UI.

When the session store collects the full tab data for a browser, it always includes the values for isPrivate and desktopMode, therefore we now include those values in the basic session store data we use in initialising a new tab object, too.

Review commit: https://reviewboard.mozilla.org/r/54424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54424/
Attachment #8755138 - Flags: review?(s.kaspari)
Comment on attachment 8755138 [details]
MozReview Request: Bug 1274390 - Include isPrivate and desktopMode in the session store data used for initialising new tabs. r=sebastian

https://reviewboard.mozilla.org/r/54424/#review51146
Attachment #8755138 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 6

2 years ago
Comment on attachment 8755138 [details]
MozReview Request: Bug 1274390 - Include isPrivate and desktopMode in the session store data used for initialising new tabs. r=sebastian

Approval Request Comment
[Feature/regressing bug #]: Mobile session store, bug 1229259
[User impact if declined]: Occasionally, restoring tabs from the Recent Tabs panel will fail.
[Describe test coverage new/current, TreeHerder]: manual (see STR in comment 2).
[Risks and why]: Low: Session store data for new tabs is basically initialised in two steps - preliminary data is added during tab creation and the full data after the DOMTitleChangedEvent during page load. This patch ensures that certain properties which are expected to be always present in the data are initialised during tab creation.
[String/UUID change made/needed]: none
Attachment #8755138 - Flags: approval-mozilla-beta?
Attachment #8755138 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 7

2 years ago
I should mention that my tabs actually _did_ end up opening, but only after several hours. (Note also that I have several hundred tabs.) I assume that doesn't change the outcome of this bug?

In any case, looks like *something* got fixed, so that's nice. \o/
(Assignee)

Comment 8

2 years ago
Hmm, that sounds most weird and to be honest I don't really have an explanation for what caused *that* behaviour (although it's nice that you didn't loose your open tabs in the end). But comment 2 still seems the most likely explanation for how we got to "org.json.JSONException: No value for isPrivate" and the tab initialisation needed fixing in any case.
(Assignee)

Comment 9

2 years ago
Whoops, I meant to say comment 1 above.
Comment on attachment 8755138 [details]
MozReview Request: Bug 1274390 - Include isPrivate and desktopMode in the session store data used for initialising new tabs. r=sebastian

Recent regression in session restore. Beta47+, Aurora48+
Attachment #8755138 - Flags: approval-mozilla-beta?
Attachment #8755138 - Flags: approval-mozilla-beta+
Attachment #8755138 - Flags: approval-mozilla-aurora?
Attachment #8755138 - Flags: approval-mozilla-aurora+
Hi Andrei, could you please plan some focused testing around session restore for 47.0b9? Thanks!
Flags: needinfo?(andrei.vaida)
 https://hg.mozilla.org/releases/mozilla-beta/rev/46eb9f7807f3
status-firefox47: affected → fixed
(Reporter)

Comment 14

2 years ago
Hmm, okay.

Is it possible that the tabs got opened (but not displayed in the Java UI), then Firefox got killed and when it was reopened, the "open" tabs were properly restored? I have the browser set to restore tabs, FWIW.
(Reporter)

Updated

2 years ago
Flags: needinfo?(jh+bugzilla)
(Assignee)

Comment 15

2 years ago
That seems plausible. It is possible/probable that session restoring got turned off temporarily if you've crashed at least twice in a row (see bug 1263110). Since the only thing that was broken here was opening the tabs in the UI, the session store was still keeping track of those invisible tabs.

Once you manage a background - foreground - background cycle without crashing, session restoring is turned back on, so your tabs will be restored normally the next time you launch Firefox.
Flags: needinfo?(jh+bugzilla)
(Reporter)

Comment 16

2 years ago
Sounds correct! Thanks for the speedy response.

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/67e6237c8895
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/b01ee9309ba0
status-firefox48: affected → fixed
(In reply to Ritu Kothari (:ritu) from comment #11)
> Hi Andrei, could you please plan some focused testing around session restore
> for 47.0b9? Thanks!

I believe this is Android only, forwarding this ni? to Ioana.
Flags: needinfo?(andrei.vaida) → needinfo?(ioana.chiorean)

Comment 20

a year ago
We have covered this with a test and I did not reproduce it. 
I will try to keep a big profiles ( hundreds of tabs to see if in the future will happen again)
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
status-firefox48: fixed → verified
status-firefox49: fixed → verified
Flags: needinfo?(ioana.chiorean)
You need to log in before you can comment on or make changes to this bug.