996590 - Tabs are not restored if Firefox is closed with QuitNow addon

Status: RESOLVED DUPLICATE of bug 995138

(Firefox for Android Graveyard :: General, defect)

Description

[u421692]

Environment:
Build: Firefox 29 Beta 8, Nightly 31.0a1(2014-04-14)
Device: Alcatel One Touch 8008D(Android 4.1.2)/ASUS Transformer Pad TF300T (Android 4.1.1)/ZTE Grand X IN (Android 4.0.4) 

Steps to reproduce:
1. Open Menu->Settings->Customize->Tabs and select "Always restore"
2. Install QuitNow addon https://addons.mozilla.org/en-US/android/addon/quitnow/
3. Have some tabs opened
4. Open Menu->Quit
5. Open Firefox

Expected result:
Tabs are restored.

Actual result:
Tabs are not restored.

I see the following error in the logcat:
E/GeckoConsole(12505): [JavaScript Error: "privateData.windows[0] is undefined" {file: "jar:jar:file:///data/app/org.mozilla.firefox_beta-1.apk!/assets/omni.ja!/components/SessionStore.js" line: 420}]

Comment 1

[u421692]

Probably affected by some of this latest changes:
http://hg.mozilla.org/mozilla-central/filelog/5b6e82e7bbbf/mobile/android/components/SessionStore.js

Comment 2

[Mark Capella [:capella]]

This won't be specific to QuitNow ... the addon functions by issuing the same "Browser:Quit" message generated by the original MainMenu option ... while mostly disabled, we still provide it for users with pre-ICS or "tv" devices.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=10e08afa522a#2168

Comment 3

[Cristina Madaras, QA [:CristinaM]]

Regression window:

Last good revision: dedf12c4e805 (2014-02-07)
Last good revision: cafe909f7e07 (2014-02-08)

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dedf12c4e805&tochange=cafe909f7e07

Comment 4

[Aaron Train [:aaronmt]]

Without looking at inbound builds, I don't see anything really standing out from that range.

Comment 5

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

Brian - Is this related to the Guest Browsing issue too?

Comment 6

[u421692]

(In reply to Mark Capella [:capella] from comment #2)
> This won't be specific to QuitNow ... the addon functions by issuing the
> same "Browser:Quit" message generated by the original MainMenu option ...
> while mostly disabled, we still provide it for users with pre-ICS or "tv"
> devices.
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java?rev=10e08afa522a#2168

The Quit option is hidden in latest Nightly, Firefox 32 is unaffected. Should I close this issue as WFM, since it's not a najor issue,or should we keep tracking for affected versions?

Comment 7

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

I am not too worried about issues that happen because of QuitNow specifically, but I was worried that this might be a deeper bug. It may be that QuitNow is just exposing a dormant bug, which should be fixed.

That's why I want Brian to take a look. The JS error could be part of the issue too.

Comment 8

[u421692]

2014-02-06 - good
2014-02-07 - bad
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e9f169c9715&tochange=dedf12c4e805

After bisecting I've found the regression:
The first bad revision is:
changeset:   167183:bf45888656ad
user:        Brian Nicholson <[e-mail]>
date:        Wed Feb 05 23:47:51 2014 -0800
summary:     Bug 965137 - Synchronously write session on application-background. r=mfinkle

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Mihai Pop from comment #6)
> The Quit option is hidden in latest Nightly, Firefox 32 is unaffected.
> Should I close this issue as WFM, since it's not a najor issue,or should we
> keep tracking for affected versions?

Not sure what you mean by Quit being hidden -- I just installed QuitNow on the latest Nightly and see the Quit button.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Brian - Is this related to the Guest Browsing issue too?

Yeah, this is essentially a dupe of bug 995138 for the reasons described in https://bugzilla.mozilla.org/show_bug.cgi?id=995138#c1. Guest mode and Quit both use System.exit(), so we don't flush anything to disk.

Bug 965137 made things worse by also relying on flushing to disk in onPause, but the session isn't the only thing affected; prefs and the disk cache can also be lost if we don't quit cleanly. The fix for these issues is to make sure our System.exit() codepath(s) first send an application-background event to Gecko.

(In reply to Mihai Pop from comment #0)
> I see the following error in the logcat:
> E/GeckoConsole(12505): [JavaScript Error: "privateData.windows[0] is
> undefined" {file:
> "jar:jar:file:///data/app/org.mozilla.firefox_beta-1.apk!/assets/omni.ja!/
> components/SessionStore.js" line: 420}]

I only see this error when I hit Quit shortly after opening Fennec. I also see this in browser.js:
E/GeckoConsole( 7732): [JavaScript Error: "win is undefined" {file: "chrome://browser/content/browser.js" line: 3380}]

If we trigger Quit very early during startup, we might not get a handle on a browser window. I don't think this error is much concern.

Comment 11

[u421692]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> (In reply to Mihai Pop from comment #6)
> > The Quit option is hidden in latest Nightly, Firefox 32 is unaffected.
> > Should I close this issue as WFM, since it's not a najor issue,or should we
> > keep tracking for affected versions?
> 
> Not sure what you mean by Quit being hidden -- I just installed QuitNow on
> the latest Nightly and see the Quit button.

The Quit button is not displayed on ICS+ devices:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=10e08afa522a#2168

On pre-ICS devices the behaviour is the same, and there are two Quit buttons(the default one, and QuitNow add-on button), so I don't know if this add-on is useful anymore.

Comment 12

[Brian Nicholson (:bnicholson)]

(In reply to Mihai Pop from comment #11)
> The Quit button is not displayed on ICS+ devices:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java?rev=10e08afa522a#2168

Yes, we removed the Quit button for ICS+ way back in bug 800071 (Fx20). That's the reason this add-on is useful -- it adds the button back for people who want it in ICS+. QuitNow isn't meant to be used on pre-ICS since those devices already show the Quit button.