Fennec Nightly 48.0a1 (2016-04-20) It seems this issue occurs since some days ago. Step to reproduce: 1. Enable "Clear private data on exit" and "Browsing history". 2. Visit some websites. 3. Tap "Quit". Actual result: History data remained, wasn't cleared. Expected result: History data should be cleared when we tap "Quit".

I can replicate this. Marking it as new, and annoying.

This WFM with an N7/6.0.1.

My device: * Android 5.0.1 * SE Android - Enforcing * Samsung Galaxy S4 I tried below steps: 1. Uninstalled Fennec Nightly. 2. Reinstalled Fennec Nightly (49.0a1 2016-05-01). 3. Followed the steps to reproduce as per comment 0. 4. Confirmed that history could be removed successfully.(?) 5. Signed in Firefox Sync. 6. Confirmed that history couldn't be removed.(?) 7. Retried removing. 8. Confirmed that history could be removed successfully.(?) There is no issues for me now, so I will change the status of this bug. Thank you for cooperation!

I've been able to replicate this with one added step. Under Settings > Advanced > Restore tabs choose don't restore after quitting nightly.

Confirmed ... Observed in logcat: GeckoConsole: [JavaScript Error: "TypeError: normalData.windows[0] is undefined" {file: "jar:jar:file:///data/app/org.mozilla.fennec-2/base.apk!/assets/omni.ja!/components/SessionStore.js" line: 666}] GeckoConsole: [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "normalData.windows[0] is undefined" {file: "jar:jar:file:///data/app/org.mozilla.fennec-2/base.apk!/assets/omni.ja!/components/SessionStore.js" line: 666}]'[JavaScript Error: "normalData.windows[0] is undefined" {file: "jar:jar:file:///data/app/org.mozilla.fennec-2/base.apk!/assets/omni.ja!/components/SessionStore.js" line: 666}]' when calling method: [nsISessionStore::removeWindow]" {file: "chrome://browser/content/browser.js" line: 1301}] Complaining code is [0] Removing that bit of logging resolves the issue. Regressing chgset is [1]. [0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js?rev=ec981b82951f&mark=665-666#663 [1] https://hg.mozilla.org/mozilla-central/rev/ec69a0c8c6ef

Sorry.

There is no window in the normalData when clearing browsing history on quit with tab restore set to "never restore", which breaks the assumption made in the logging function. Review commit: https://reviewboard.mozilla.org/r/49921/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49921/

Comment on attachment 8747546 [details] MozReview Request: Bug 1266594 - Check normalData.windows[0] exists before accessing tab count. r=margaret bah, it seems while your bit was an issue, (and this corrects it), it was masking something further. In testing your patch, I see this bug |Clear private History data| recurs random orange / intermittently with the logcat messages below ... This seems to also explain new |Bug 1269159 - Clear private data on exit is incomplete|, where the issue is |search history|. Both depend on "Sanitize:ClearHistory" messages, which BrowserApp is registering/unregistering on the Java main thread, while observing on the GeckoThread, and Gecko is losing the race in a good number of cases. This isn't Gecko/Text Selection related, so I'm forwarding to margaret for disposition, but suggest we could close your patch on this bug and resolve the bigger issue on bug 1269159 (??) 05-01 17:18:10.535 29037 29061 E GeckoConsole: [JavaScript Error: "Java-side search history clearing failed: No listeners for request" {file: "resource://gre/modules/Sanitizer.jsm" line: 172}] 05-01 17:18:10.535 29037 29061 E GeckoConsole: Sanitizer.prototype.items.searchHistory.clear/<@resource://gre/modules/Sanitizer.jsm:172:23 05-01 17:18:10.535 29037 29061 E GeckoConsole: [JavaScript Error: "Java-side history clearing failed: No listeners for request" {file: "resource://gre/modules/Sanitizer.jsm" line: 146}] 05-01 17:18:10.535 29037 29061 E GeckoConsole: Sanitizer.prototype.items.history.clear/<@resource://gre/modules/Sanitizer.jsm:146:23

Comment on attachment 8747546 [details] MozReview Request: Bug 1266594 - Check normalData.windows[0] exists before accessing tab count. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49921/diff/1-2/

Since there's more to this bug as per comment 8, I've split off the session store logging issue.

Since bug 1270011 addresses the regression, this bug doesn't need to track a release. However, let's try to investigate more to figure out what's going wrong with clear private data.

The behaviour that I reported under 1269833 (a duplicate of this bug) in respect of Firefox for Android Nightly 49.0a1 (2016-05-03) now appears fixed in 49.0a1 (2016-05-07). Clear private data on exit works fine with history being cleared on exit further to being set in the Settings>Privacy menu.

The bug report I submitted (1294931) was marked as a duplicate but I don't believe it is, while it does involve using clear data on exit, the issue isn't history not being cleared, the issue is add-on specific data along with a few other things not being saved on quit.

Im running latest beta (android) and this big (not clearing anything from my 'clear private data on exit' list) appears since at least ten updates. I dont get how this cant be fixed, but i have no clue of programming either.

It feels like there are more reports coming in lately. Re-flagging this to discuss in triage.

Atleast something. How come this big privacy bug is still present after this amount of time?

Also having this issue for some months now and have had to clear my data manually. Mostly commenting to subscribe to bug.

Hi Firefox crew, Just checking in here 4 months later, and adding that it's gotten worse now. With a complete reinstall on the same model of phone (LG G4), Android 6.0, Kernel 3.10.84-g7601311 lge@android-build, and Firefox 49.0: Upon quit, it not only does not clear my private settings, when firefox is relaunched, <i>it opens up all the pages again</i>. I'm thinking with such bad behavior, there's a potential security issue here, and a possibility of stealing logon credentials. I don't mind being wrong on this. I hope I'm wrong on this. So, I hate to press, but is there an ETA? Maybe I'm alone here, but I would like to know if I need to make alternate browser arrangements.

(In reply to dpf from comment #26) > Upon quit, it not only does not clear my private settings, when firefox is > relaunched, <i>it opens up all the pages again</i>. *That* is not this bug, it's the intended default behaviour since Firefox 48. It can however be configured under Settings -> Advanced -> Restore tabs. Also, bug 1275662 might be relevant there.

Richard, dpf: Is this working for you as expected after setting "Advanced -> Restore tabs" to "Don't restore after quitting Firefox"?

The 'don't restore tabs' feature is working as expected. The history is just not being cleared.

OK, thanks for pointing me to that new, uh, feature? :) Yes, when I turn off the restore tabs (that I never chose to enable) it clears everything, including history. I wish my other phone hadn't bricked itself because it was saving random items, sometimes history, sometimes top sites, sometimes all - this was mentioned in the bug I submitted a few months ago. The BIG question: Why is restore tabs secretly overriding my settings to clear all the private data? Even if this does get documented and the bug becomes a feature, this is not intuitive behavior at all. Restore tabs should never have been defaulted to on without explicitly telling the user.

Just checked with latest aurora(?) and without any addons - still not working. Wow..

Bug 1275662 related?

+1 to the complaints about Firefox for Android not clearing history even when "clear private data on exit" is checked.

Bumper - any news on this?

To anybody who has been experiencing this: a) Can you still reproduce this on Nightly? b) If yes, does this build (https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-8f9237b325a92e72534d5ebfef6abc5125ddf2a2/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk) fix the issue?

Hi Jan, a) yes, can be reproduced on Nightly. b) yes, issue is not present in given build. At least on my device. Best regards

(In reply to Jan Henning [:JanH] from comment #36) > To anybody who has been experiencing this: > a) Can you still reproduce this on Nightly? Yes, on my Nexus 4 device (but not on my Xperia Z3 Compact, so far at least) > b) If yes, does this build > (https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de- > 8f9237b325a92e72534d5ebfef6abc5125ddf2a2/try-android-api-15/fennec-53.0a1.en- > US.android-arm.apk) fix the issue? Yes. From the home screen when I select "Quit" from the menu I see all the topsites tiles return to the stock defaults (Facebook, Youtube, etc.) and then fennec exits. However there is a delay of maybe 1-2 seconds between the tiles changing and fennec exiting - it's just a tad too long, makes me wonder if fennec will quit at all. If we can shorten that it would be perfect.

(In reply to Jan Henning [:JanH] from comment #36) > a) Can you still reproduce this on Nightly? Sometimes yes, but not always (I use Samsung Galaxy S4). > b) If yes, does this build > (https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de- > 8f9237b325a92e72534d5ebfef6abc5125ddf2a2/try-android-api-15/fennec-53.0a1.en- > US.android-arm.apk) fix the issue? Apparently yes, so far. Thank you so much!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #38) > Yes. From the home screen when I select "Quit" from the menu I see all the > topsites tiles return to the stock defaults (Facebook, Youtube, etc.) and > then fennec exits. However there is a delay of maybe 1-2 seconds between the > tiles changing and fennec exiting - it's just a tad too long, makes me > wonder if fennec will quit at all. If we can shorten that it would be > perfect. The solution implemented is just simply waiting for Gecko to exit before shutting down the Java side in order to avoid any race conditions, hence the added delay. I'd noticed it myself, but I wanted to get some feedback if this was actually working and didn't want to wait for a new try build to complete. In any case this build here should fix the delay issue (and hopefully not break anything else) by sending Firefox into the background while waiting for Gecko to exit: https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-11a5665e5cb9dd36b5132803ee20dd21ee4239bc/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk

Comment on attachment 8822480 [details] Bug 1266594 - Part 1 - Wait for Java-side sanitizing to be triggered before exiting the UI. https://reviewboard.mozilla.org/r/101382/#review101936 Thank you Jan! :)

(In reply to Jan Henning [:JanH] from comment #40) > In any case this build here should fix the delay issue (and hopefully not > break anything else) by sending Firefox into the background while waiting > for Gecko to exit: > https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de- > 11a5665e5cb9dd36b5132803ee20dd21ee4239bc/try-android-api-15/fennec-53.0a1.en- > US.android-arm.apk Works great, thanks!

(In reply to Jan Henning [:JanH] from comment #40) > (and hopefully not break anything else) Actually that second try build still has some potential issues with the session store which could cause your open tabs to get lost on quitting, but that should be fixed for the version that's up for review now.

JanH: if this approach lands, please please please write it up in in-tree docs: this kind of lifecycle stuff is very difficult for people to apprehend from reading code itself, and the knowledge will eventually be lost and have to be rediscovered by observation. Also, what happens if the app is foregrounded again at various points in this messaging lifecycle? That is, if Gecko takes five seconds to flush session store, ten seconds to shut down, and the browser DB takes five more seconds to delete everything, what happens if I click an external link at 3, 7, or 12 seconds?

(In reply to Richard Newman [:rnewman] from comment #46) > JanH: if this approach lands, please please please write it up in in-tree > docs Thanks for the reminder, will do. > Also, what happens if the app is foregrounded again at various points in > this messaging lifecycle? That is, if Gecko takes five seconds to flush > session store, ten seconds to shut down, and the browser DB takes five more > seconds to delete everything, what happens if I click an external link at 3, > 7, or 12 seconds? Hmm, you're right, this creates a window of time where the UI is backgrounded but still alive, while Gecko is already shutting down. So the UI might foreground itself again and open a new tab, but the session store will not save it if it's already flushed its data and Gecko just continues shutting down, closing the UI again once it finally exits. Thinking about it, a better - although probably still not perfect solution - would be to delay the call to doShutDown() only until Gecko has messaged the UI's sanitize handlers - at least for me, this is happening noticeably sooner than waiting for Gecko:Exited.

Okay, this build calls doShutDown() as soon as Gecko has messaged all Java-side sanitize handlers and is sending "Sanitize:Finished", which should - be a bit quicker than waiting for Gecko to exit completely and therefore no longer require the hack of backgrounding the app while waiting for Gecko to exit - reduce the window of time where Gecko is effectively shutting down behind the UI's back and externally opened tabs could get lost - prevent the potential background-foreground-background cycle caused by an external tab arriving, because the UI now remains in the foreground until it is shutting down for good Therefore, if anybody who can easily reproduce this please test whether with this build (https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de-095fa2abee3b9c04de87f3d957ad3973dd9c6545/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk) a) this bug remains fixed? b) the delay between hitting the quit button and the UI exiting is acceptable?

> Therefore, if anybody who can easily reproduce this please test whether with > this build > (https://archive.mozilla.org/pub/mobile/try-builds/mozilla@buttercookie.de- > 095fa2abee3b9c04de87f3d957ad3973dd9c6545/try-android-api-15/fennec-53.0a1.en- > US.android-arm.apk) > a) this bug remains fixed? Yes. > b) the delay between hitting the quit button and the UI exiting is > acceptable? Yes (for me). Thank you very much.

I also confirm the build from comment 48 works well. Thanks!

@rnewman: Referring to the proposed docs, if an external tab was to arrive before between 1) and 5), it'll probably get lost - with the reworked patch, the session store hasn't yet flushed its data, but Gecko is probably busy running BrowserApp.quit() anyway, so the whole process of opening a new tab and telling the session store about it only gets to run somewhere after 7), when the session store has already flushed its data to disk. If on the other hand the external tab arrives after 5), i.e. after the UI has started closing, it'll just start up Firefox again. Empirically it seems that starting the UI up again while Gecko is still shutting down can sometimes leave us in a borked state, but *that* can equally happen with the current code as well, so at least we shouldn't be any worse off there.

Comment on attachment 8822481 [details] Bug 1266594 - Part 2 - Fix the session store to handle the new shutdown sequence. https://reviewboard.mozilla.org/r/101384/#review102898 LGTM

Comment on attachment 8822480 [details] Bug 1266594 - Part 1 - Wait for Java-side sanitizing to be triggered before exiting the UI. https://reviewboard.mozilla.org/r/101382/#review102988 ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:643 (Diff revision 2) > > } else if (event.equals("Gecko:Exited")) { > // Gecko thread exited first; let GeckoApp die too. > doShutdown(); > > + } else if (event.equals("Sanitize:Finished")) { nit: `"Sanitize:Finished".equals(event)`. and change `"Gecko:Exited"` above as well. ::: mobile/android/chrome/content/browser.js:1469 (Diff revision 2) > promises.push(Sanitizer.clearItem(key)); > } > } > > Promise.all(promises).then(function() { > Messaging.sendRequest({ Change to `GlobalEventDispatcher.sendRequest` ::: mobile/android/chrome/content/browser.js:1479 (Diff revision 2) > > if (callback) { > callback(); > } > }).catch(function(err) { > Messaging.sendRequest({ Same

Comment on attachment 8823400 [details] Bug 1266594 - Part 3 - Explain shutdown sequence and related data cleaning problems in the docs. https://reviewboard.mozilla.org/r/101930/#review102990

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/567b86317b84 Part 1 - Wait for Java-side sanitizing to be triggered before exiting the UI. r=jchen https://hg.mozilla.org/integration/autoland/rev/cd8da5953744 Part 2 - Fix the session store to handle the new shutdown sequence. r=sebastian https://hg.mozilla.org/integration/autoland/rev/55f17179f57f Part 3 - Explain shutdown sequence and related data cleaning problems in the docs. r=jchen

https://hg.mozilla.org/mozilla-central/rev/567b86317b84 https://hg.mozilla.org/mozilla-central/rev/cd8da5953744 https://hg.mozilla.org/mozilla-central/rev/55f17179f57f

Can this be uplifted to Aurora & Beta, and possibly ESR ?

ESR is not supported on Android as far as I'm aware and even if it was, uplifting one version would be enough to get this into ESR52. Aurora should certainly be possible, Beta might be a bit late, especially since this needs dedicated patches because of the event listener changes.

(In reply to Jan Henning [:JanH] from comment #54) > Empirically it seems > that starting the UI up again while Gecko is still shutting down can > sometimes leave us in a borked state, but *that* can equally happen with the > current code as well, so at least we shouldn't be any worse off there. That's my principal concern: last I checked (and my knowledge might well be out of date!) we did *not* wait for Gecko to shut down when we quit, so the window of time right now is ~0. With your patch it might be significant, and thus we might introduce another bug by fixing this one. Thoughts?

Aurora is still possible, I think it's a bit late for beta 51 though. Do you want to try it this week with 52, and we could ask QE for some testing?

Ioana, can you try testing around this issue in nightly ? It seems worth verifying the fix and doing some exploratory testing around session store and clearing private data. What do you think?

@Liz Yes, Aurora will be fine - I'll put up the uplift request later together with the updated patch for Part 1.

(In reply to Richard Newman [:rnewman] from comment #66) > Thoughts? Yes, although I'm a bit puzzled why you're quoting that paragraph of mine, which doesn't quite seem to fit the concerns you voice below. Referring to the quoted paragraph, I'll stand by my opinion. As far as GeckoApp is concerned, there seem to be mainly two phases - before calling doShutdown(), and after [1]. If an intent to open an external tab arrives before this, the UI will open a new tab and also attempt to open that tab in Gecko as well, which might or might not work (see below). If the intent arrives afterwards, it'll just start up a new instance of Firefox. In this latter case, we seem to have a problem in that 1. we don't quite handle the case of starting a new instance of GeckoApp while the previous is just shutting down 2. or we don't quit handle the case of starting a new instance of GeckoApp while the previous Gecko instance is about to exit 3. or ??? Be that as it may, we can apparently end up in a state where the UI starts back up, but Gecko fails to load and no web content renders. *This* particular bug doesn't depend on my patch - by sending intents via ADB from the command line with varying timings relative to pressing the Quit button, I've been able to trigger this on Nightly builds without my patch as well. If (if) theory 2. is correct, this patch might even actually help here by reducing the amount of time Gecko is running without a corresponding GeckoApp instance, but that is just pure speculation. > That's my principal concern: last I checked (and my knowledge might well be > out of date!) we did *not* wait for Gecko to shut down when we quit, so the > window of time right now is ~0. > > With your patch it might be significant, and thus we might introduce another > bug by fixing this one. Now regarding this concern, this seems more related to the fact that there can now be a prolonged period of time (although not quite as long as waiting for Gecko to fully exit) where an incoming external tab could be lost because the UI is still running normally while Gecko is already busy preparing shutdown. Any tabs that the UI attempts to open while BrowserApp.quit() is being handled are only processed after the application-quit notification, at which point the session store has already flushed its data and stopped doing new writes, so the tab gets lost. That is indeed something that has been introduced (or at the very least magnified) by this patch. Provided that opening new tabs is indeed still successful at that stage, my concern was that I've no idea in how far it is likely that another write by the session store at that stage will be forcibly interrupted because of Gecko exiting. However thinking some more about it, the new session store backups should make this possible in a safe manner: Clearing history on quitting runs browser:purge-session-history, which kills off the backup session copy, forgets any recently closed tabs, and then writes that new state to disk. With a little tweak, the next write after that (e.g. a new tab arriving) will first synchronously recreate the backup copy. If that fails, we've always got the original session file, otherwise we'll have the backup file in place to handle any failures while writing out the new tab (which in turn first writes to a temp file), so at least we should be sort of guaranteed not to loose the current session while making an attempt to write out the additional tab. I'll file additional bugs for these things later. [1] I don't profess any deep knowledge on that subject, though. I've no idea for example how atomic the call to doShutdown() actually is, or whether an incoming intent can get lost or cause other strange things purely on the Java side if it arrives at just the right/wrong moment in relation to doShutdown().

Approval Request Comment [Feature/Bug causing the regression]: Clearing history on quitting [User impact if declined]: Browsing history may fail to clear when quitting Firefox even though the corresponding option is selected. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: No, although two people verified it on a try build containing the same fix. [Needs manual test from QE? If yes, steps to reproduce]: 1. Create some browsing history. 2. Enable Settings -> Privacy -> Clear private data on exit -> Browsing history 3. Quit Firefox from the menu. (Note: Because this bug is due to a race condition, reproducibility might vary and also be device-dependent) [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: Not much. [Why is the change risky/not risky?]: Only small tweaks to the shutdown sequence between the UI and Gecko. Biggest unknown is how probable history clearing taking significantly longer than usual can be, which could delay the shutdown of the UI after hitting the Quit button - should any new tabs (e.g. from external apps) arrive just within that intervening period they might be lost in the shutdown process. [String changes made/needed]: none

At least for me, hg import didn't want to apply the whole patch automatically, so just to be on the safe side, here's a dedicated patch for Part 2 as well, even though it should be identical to the code that's on central. Approval Request Comment [Feature/Bug causing the regression]: see Part 1 [User impact if declined]: Part 1 might possibly change some event ordering for the session store on shutdown, which can lead to the current session being lost if the user enabled history cleaning on shutdown. [Is this code covered by automated tests?]: The shutdown bits no. [Has the fix been verified in Nightly?]: see Part 1 [Needs manual test from QE? If yes, steps to reproduce]: When clearing the browsing history on exit with tab restoring set to "Always restore", the currently open tabs shouldn't be lost. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: No [Why is the change risky/not risky?]: No major changes in the main session store code, only some small changes in shutdown handling. [String changes made/needed]: none

Comment on attachment 8825514 [details] [diff] [review] Bug 1266594 - Part 1 - AURORA.patch fix race condition when clearing history on fennec shutdown, aurora52+

Comment on attachment 8825518 [details] [diff] [review] Bug 1266594 - Part 2 - AURORA.patch fix race condition when clearing history on fennec shutdown, aurora52+

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b1e59d764d5 https://hg.mozilla.org/releases/mozilla-aurora/rev/b2bd1a80baa5 https://hg.mozilla.org/releases/mozilla-aurora/rev/e7d445c74aa1

Verified as fixed in build 52.0b1; Device: Huawei MediaPad M2 (Android 5.1.1) and Samsung Note 4 (Android 5.0.1). Tested in both situations: Always restore and don't restore tabs. The browsing history is cleared.

It seems like the bug returned to aurora. Since three updates (or more) it doesnt clear private data/history.

Please open a new bug if you're seeing this again.

Scratch that, I'm opening it myself.

This was verified by Sorina in Comment #76 Depending bugs were also fixed and verified.

Bug is back on 57. Fast shutdown without clearing.