1244650 - Failure to clear Forms and Search Data on exit when configured to

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: Leak data regression

+++ This bug was initially created as a clone of Bug #1244525 +++

Build Identifier:
https://hg.mozilla.org/releases/mozilla-aurora/rev/f05cc5ce1dc7ca7305df5598e7c40386c090b6c2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 ID:20160131004012

This is regression since 46.

Reproducible: always

Steps to reproduce:

1. Open about:preferences#privacy 
   Select "Use custom settings for history"
   Check "Clear history when Firefox closes" and then click "Setting"
   Check "Form & Search History", if necessary

2. Type "aaaa" and ENTER in serchbar or any other input field w/autocomplete
3. Quit Browser and restart
4. Type "a"
   --- observe form history data

Actual results:
The data was not cleared

Expected results:
The data should have been cleared.

Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c3c230a896320099bc14a1ef4b78e41ecc657b1f&tochange=ea9e749bb86bfb584e8591959ac57278bf43c0e4

Suspect:bug 1211849

Comment 1

[Marco Bonardo [:mak]]

ugh, we only removed the check, not the clearing.
I can take a look, I really don't see how the change could make a difference...

Comment 2

[Marco Bonardo [:mak]]

The only theory I have so far is that canClear was throwing so that sanitization was actually happening at the next startup.
If I try to reproduce I see that formData is cleared, but when we run there's no window around, so it's unlikely we can clear session history data.
Though bug 1243549 seems to point out also sanitize on startup should have failed.

There's definitely something broken at the times we startup and shutdown the sanitizer, if we expect it to act on windows but we invoke it when there are no windows.

Comment 3

[Marco Bonardo [:mak]]

Attachment: Errors on shutdown

Comment 4

[Marco Bonardo [:mak]]

I cannot reproduce the data leak AFTER the startup, that means my profile was sanitized properly on startup.

Definitely there's a problem on shutdown where FormData cannot create an async statement. I suspect the service already shutdown or such, probably canClear was hiding an issue here. Didn't investigate in form data code yet.

Startup could fail if it hits the tabBrowser undefined case of bug 1243549, in such a case we'd bail out and skip form data clear. This is also wrong, the 2 clearings should be isolated so that one doesn't prevent the other one.

I wonder if we always failed to clear form data on shutdown but always did on startup, and this bug is just due to some timing issues that now makes us throw in the first part of the Formdata sanitizer.

Comment 5

[Benjamin Kerensa [:bkerensa]]

Tracking since this is a regression and please request uplift when this is fixed

Comment 6

[Marco Bonardo [:mak]]

Attachment: MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

Review commit: https://reviewboard.mozilla.org/r/33869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33869/

Comment 7

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba010d11a65d

Comment 8

[Marco Bonardo [:mak]]

Just a couple words about the issue.

Basically when we moved Places to async shutdown, its shutdown has been simplified from 2 phases (profile-change-teardown, profile-before-change) to a single phase (profile-before-change).
Ideally in the async shutdown world that works, cause we have blockers, so we CAN ensure clients complete shutdown before Places without using multiple notifications.

The problem is that some of the clients (Sanitizer for example) don't just depend on Places, but also on a bunch of other components, that also shutdown at profile-before-change.
For this reason the old shutdown procedure had 2 phases, so that we could ensure clients work started before profile-before-change. Unfortunately I overlooked this issue when we changed Places shutdown.

What happened in practice is that we moved sanitization from profile-change-teardown to profile-before-change, but since we cannot predict that it will be the first observer of profile-before-change, at that point some components (form history) could have shutdown already.

This patch restores the 2 phases Places shutdown, with clients closing at teardown and connection closing at before-change.
In future we could further improve the situation removing some of the shutdown notifications for actual modern shutdown blockers.

Comment 9

[Marco Bonardo [:mak]]

I also did changes to sanitize, to make it start all the jobs at the same time and wait later, otherwise we'd end up serializing every single sanitization on the previous one, and it takes far more time.
This should speed up the process quite a bit.
But we may not be able to observe the benefit through telemetry cause we somehow broke the total measurement in some previous patch, it was measuring the last item instead of all the items... that's sad cause if we'd have not broke it, it could probably have raised a warning about sanitization taking far more time than usual.

Comment 10

[p.r.gotch]

Will this fix get released in Firefox 45? The meta data on this bug is still wrong in that it claims it doesn't affect 45 when in fact this has been broken for far longer?

Comment 11

[Loic]

(In reply to paulg from comment #10)
> Will this fix get released in Firefox 45? The meta data on this bug is still
> wrong in that it claims it doesn't affect 45 when in fact this has been
> broken for far longer?

It's bug 1244525, I think.

Comment 12

[Marco Bonardo [:mak]]

(In reply to paulg from comment #10)
> Will this fix get released in Firefox 45? The meta data on this bug is still
> wrong in that it claims it doesn't affect 45 when in fact this has been
> broken for far longer?

It would be nice to uplift, but there's other bugs that should land there first, like bug 1243549. It depends on the schedule. we can uplift unless we get too close to release.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Thanks for the patch. I haven't had time to look at it today, but I'll try and review this tomorrow.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8716537 [details]
MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

https://reviewboard.mozilla.org/r/33869/#review31025

Good work. Sanitize is finally starting to look like readable code.

Given our recent experience with "what the heck were we thinking when we refactored this that way?", I'd appreciate if you could amend the commit message to explain in as many lines as it takes what this patch does and, more importantly, why it does it.

::: browser/base/content/sanitize.js:159
(Diff revision 1)
> +      let item = this.items[itemName];

You removed the check for `"clear" in item`. I don't remember what it was for, but could you tell me why?

::: browser/base/content/sanitize.js:168
(Diff revision 1)
> +    // Don't use Promise.all() since it would stop at the first rejection.

Well, you could make it

```js
for (...) {
  try {
    handles.push(item.clear(range).catch(ex =>
      annotateError(itemName, ex)
    ))
  } catch (ex) {
    ...
  }
}

Promise.all(handles);
```

right? I think it would be a tad simpler. If you wish to keep this version of the code, I'm fine with that, but if you do this, please rephrase the comment about `Promise.all`.

::: browser/base/content/sanitize.js:179
(Diff revision 1)
> +    TelemetryStopwatch.finish("FX_SANITIZE_TOTAL", refObj);

So you're changing again the meaning of `FX_SANITIZE_TOTAL`? I guess having total time makes sense, even if it's async, since it's done on startup/shutdown. Please add a comment to that effect in the code and/or in the commit message.

::: browser/base/content/sanitize.js:199
(Diff revision 1)
> -      clear: function ()
> +      clear(range) {

Not an issue, just a question: is this syntax the new standard? I personally find it makes harder to find code.

::: browser/base/content/sanitize.js:200
(Diff revision 1)
> -      {
> +        let seenException;

Good idea.

I believe that this change (e.g. "making each cleanup operation able to recover from most errors and carry on with cleaning up what can be cleaned up") should be mentioned somewhere in the commit message.

::: browser/base/content/sanitize.js:211
(Diff revision 1)
> +          seenException = ex;

Good idea.

::: browser/base/content/sanitize.js:231
(Diff revision 1)
> -      clear: Task.async(function* ()
> +      clear: Task.async(function* (range) {

Good idea.

::: browser/base/content/sanitize.js:237
(Diff revision 1)
> -        var cookieMgr = Components.classes["@mozilla.org/cookiemanager;1"]
> +        // Clear cookies.

Here, too, documenting this in the commit message would be useful.

::: browser/base/content/sanitize.js:329
(Diff revision 1)
> -        Components.utils.import("resource:///modules/offlineAppCache.jsm");
> +          Components.utils.import("resource:///modules/offlineAppCache.jsm");

Same here.

::: browser/base/content/sanitize.js:356
(Diff revision 1)
> +          TelemetryStopwatch.finish("FX_SANITIZE_HISTORY", refObj);

So you're changing the semantics of `FX_SANITIZE_HISTORY`? I get that the new semantics are closer to the name of the histogram. Could you document this in the commit message, for the sake of people who are going to ponder why `FX_SANITIZE_HISTORY` has changed suddenly?

::: browser/base/content/sanitize.js:418
(Diff revision 1)
> +          yield new Promise((resolve, reject) => {

Nit: You're not using `reject`.

::: browser/base/content/sanitize.js
(Diff revision 1)
> -          Components.utils.reportError(error);

You removed error reporting, I assume by accident.

::: toolkit/components/places/Database.cpp:405
(Diff revision 1)
> -  if (DatabaseShutdown::IsStarted())
> +  if (ConnectionShutdownBlocker::IsStarted()) {

Shouldn't this be `PlacesShutdownBlocker::IsStarted()`?

::: toolkit/components/places/Database.cpp:1641
(Diff revision 1)
> +  nsCOMPtr<mozIStorageCompletionCallback> connectionShutdown = mConnectionShutdown.forget();

A bit weird that one of them is nulled and the other one is forgotten, but ok.

::: toolkit/components/places/Database.cpp:1717
(Diff revision 1)
> -  if (strcmp(aTopic, TOPIC_PROFILE_CHANGE_TEARDOWN) == 0 ||
> +  if (strcmp(aTopic, TOPIC_PROFILE_CHANGE_TEARDOWN) == 0) {

Shouldn't this code move to the profileChangeTeardown blocker?

::: toolkit/components/places/Database.cpp
(Diff revision 1)
> -      strcmp(aTopic, TOPIC_SIMULATE_PLACES_MUST_CLOSE_1) == 0) {

Could you explain in the commit message why we don't need this topic anymore?

::: toolkit/components/places/Database.cpp:1746
(Diff revision 1)
> -  } else if (strcmp(aTopic, TOPIC_SIMULATE_PLACES_MUST_CLOSE_2) == 0) {
> +  } else if (strcmp(aTopic, TOPIC_SIMULATE_PLACES_SHUTDOWN) == 0) {

Could you add an explicit comment to emphasize the fact that this entire block is only for tests?

::: toolkit/components/places/Database.cpp:1759
(Diff revision 1)
> -      shutdownPhase->RemoveBlocker(mConnectionShutdown.get());
> +        shutdownPhase->RemoveBlocker(mClientsShutdown.get());

Shouldn't we remove the blocker *after* spinning the event loop?

::: toolkit/components/places/Shutdown.h:17
(Diff revision 1)
> +/**

Nit: giving a number to each step might make things easier to read. Also, it would make it easier to refer to this list of steps in the .cpp.

::: toolkit/components/places/Shutdown.h:22
(Diff revision 1)
> + * profile-change-teardown and forwards it to the Database singleton.

If I read `Databse::Init()` correctly, `Database` is registering to observe `profile-change-teardown` directly, no?

::: toolkit/components/places/Shutdown.h:36
(Diff revision 1)
> + * It notified places-will-close-connection notification, that is the last one

Nit: typo?

Also, you should probably mention that `places-will-close-connection` is also for legacy clients.

::: toolkit/components/places/Shutdown.h:64
(Diff revision 1)
> +  // The current state, used internally and for forensics/debugging purposes.

Some of these states don't seem to make sense to both subclasses, right?

::: toolkit/components/places/Shutdown.h:103
(Diff revision 1)
> +  // give the instances of `DatabaseShutdown` unique names.

I suspect that this comment needs to be updated.

::: toolkit/components/places/Shutdown.cpp:10
(Diff revision 1)
> +uint16_t PlacesShutdownBlocker::sCounter = 0;

You don't seem to be using the counter.

::: toolkit/components/places/Shutdown.cpp:74
(Diff revision 1)
> +  return NS_OK;

`NS_ERROR_NOT_IMPLEMENTED`?

::: toolkit/components/places/Shutdown.cpp:119
(Diff revision 1)
> +  }

According to your documentation in Shutdown.h, I was expecting the code to notify legacy clients. What am I missing?

::: toolkit/components/places/Shutdown.cpp:175
(Diff revision 1)
> +    (void)os->NotifyObservers(nullptr, TOPIC_PLACES_WILL_CLOSE_CONNECTION, nullptr);

Nit: `unused <<` instead of `void`?

::: toolkit/components/places/Shutdown.cpp:185
(Diff revision 1)
> +  // Database::Shtudown will invoke Complete once the connection is closed.

Typo: `Shutdown`.

::: toolkit/components/places/Shutdown.cpp:199
(Diff revision 1)
> +  // eventual cycles.

Nit: s/eventual/possible/

Comment 15

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/33869/#review31025

> You removed the check for `"clear" in item`. I don't remember what it was for, but could you tell me why?

Adding an item to the sanitizer involves creating an object with a clear() method, it's like an API, so I don't see why the check should ever be needed.
I did some code archeology and, when I found this comes from hg@1, I figured it's just nonsense code today. I suspect was just an overzealous check.

> Well, you could make it
> 
> ```js
> for (...) {
>   try {
>     handles.push(item.clear(range).catch(ex =>
>       annotateError(itemName, ex)
>     ))
>   } catch (ex) {
>     ...
>   }
> }
> 
> Promise.all(handles);
> ```
> 
> right? I think it would be a tad simpler. If you wish to keep this version of the code, I'm fine with that, but if you do this, please rephrase the comment about `Promise.all`.

yes, provided I convert all the clear() to being Tasks... I was already thinking of doing that honestly, so let's just do it.

> So you're changing again the meaning of `FX_SANITIZE_TOTAL`? I guess having total time makes sense, even if it's async, since it's done on startup/shutdown. Please add a comment to that effect in the code and/or in the commit message.

actually I'm not changing it, I'm fixing it.
indeed the histogram description states "description": "Sanitize: Total time it takes to sanitize (ms)"
for wahtever reason it was instead measuring single sanitize steps time.
I will add a comment to the commit message

> Not an issue, just a question: is this syntax the new standard? I personally find it makes harder to find code.

not sure what a standard is, it's shorthand methods in ES6 and I expect them being used more and more. A good ES6 highlighter like BabelJS does its job properly on these.

> You removed error reporting, I assume by accident.

The exception will go upstream and hit annotateError that is already reporting to the console. We would double-report.

> A bit weird that one of them is nulled and the other one is forgotten, but ok.

in one case we pass ownership, in the other case we just release.

> Shouldn't this code move to the profileChangeTeardown blocker?

I thought about that, but it's not really part of the shutdown procedure, this is just a bad hack we need fpr a race condition when handling notifications. I prefer to keep this closer to the notifications catching code.

> Shouldn't we remove the blocker *after* spinning the event loop?

It doesn't matter, cause here we are simulating shutdown of just Places. The blocker is for the REAL shutdown, so removing it before or after it, won't make any difference. This way the code is simpler.
I slightly reworked the comment.

> Nit: giving a number to each step might make things easier to read. Also, it would make it easier to refer to this list of steps in the .cpp.

Sorry, I don't want to fill up the code with  numbers to force someone jumping here and there like crazy. Plus it would make any future change either a nightmare to keep references correct, or would leave them uncorrect and even more confusing.
The comment at the top of the shutdown header is a global overview of what's happening, the code is not complicate enough to deserve referencing, also considered most of it is now in a separate source file.

> Some of these states don't seem to make sense to both subclasses, right?

yes, but I didn't want to split it, it looks simpler to retain it in the base class.

> You don't seem to be using the counter.

it is used in the PlacesShutdownBlocker constructor, that is invoked by each derived class.

> According to your documentation in Shutdown.h, I was expecting the code to notify legacy clients. What am I missing?

The documentation says "Database first of all checks if init was complete, to avoid race conditions, then it notifies "places-shutdown" to legacy clients", so it's Database doing that.

I could move it here, but I want to avoid going too deep into changes. The risk here is we'll move places-shutdown at a different time, where it's not expected by legacy consumers. At that point I'd prefer converting places-shutdown consumers to async shutdown and remove places-shutdown completely, rather than update them first cause places-shutdown changed, and then to async shutdown.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8716537 [details]
MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33869/diff/1-2/

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8716537 [details]
MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

https://reviewboard.mozilla.org/r/33869/#review31673

::: toolkit/components/places/Shutdown.h:17
(Diff revision 1)
> +/**

In that case, could you find another way to make this more readable? Right now, it is relatively hard to read Shutdown.h and very hard to refer to it from Shutdown.cpp.

::: toolkit/components/places/Shutdown.h:64
(Diff revision 1)
> +  // The current state, used internally and for forensics/debugging purposes.

Sure. Adding a comment in front of the steps specific to one of the paths or the other would be nice, though.

::: toolkit/components/places/Shutdown.cpp:119
(Diff revision 1)
> +  }

Fair enough.

Bonus points if you find a way to make sure that developers will not misunderstand the comment as I have.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8716537 [details]
MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

https://reviewboard.mozilla.org/r/33869/#review31733

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8716537 [details]
MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33869/diff/2-3/

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dae4f2a15d70

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dae4f2a15d70

Comment 23

[Marco Bonardo [:mak]]

Ryan, I'd like to uplift this fix cause it fixes a lot of issues with shutdown, the Sanitizer and will definitely help with bug 1248489. Since the patch is not exactly small, I think it would be nice to have a specific task for QA to verify the Sanitizer (Clear Recent History and Clear History on Shutdown) functionality.

How do you think we should handle it, would it be possible to have a quick testing session before the uplift, or better to uplift then have just a more complete QA testing session of it?

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8716537 [details]
MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

Approval Request Comment
[Feature/regressing bug #]: Sanitizer is broken from many versions, it went worse in 44 and 46, after we started fixing its crappy code... The waterfall started with bug 1089695, and got worse with further changes.
[User impact if declined]: Form data, search data, cookies (at least) are not properly cleared on shutdown. We are often crashing on shutdown (see bug 1248489) due to timeout abort in async shutdown. This patch should improve both privacy and shutdown handling.
[Describe test coverage new/current, TreeHerder]: sanity is covered by existing tests, form data clear has a new test
[Risks and why]: Risk is medium, this touches quite some code and Places shutdown, but the new code looks much nicer than the status quo. While the risk is not low, it should help a lot with shutdown crashes and sanitization. So the risk is balanced with the gains. I needinfo-ed RyanVM to setup a special testing session with QA.
[String/UUID change made/needed]: none

Comment 25

[Robert Kaiser]

Ugh, this is large. Bug 1248489 seems to be one of the issues in 45 and stability of beta is really bad right now, but this work being that large puts us between a rock and a hard place.

Comment 26

[Marco Bonardo [:mak]]

(In reply to Robert Kaiser (:[e-mail]) from comment #25)
> Ugh, this is large. Bug 1248489 seems to be one of the issues in 45 and
> stability of beta is really bad right now, but this work being that large
> puts us between a rock and a hard place.

Yes, I'm sorry, the Sanitizer was a mess, barely maintained and working by luck (just see the commit message, https://bugzilla.mozilla.org/show_bug.cgi?id=1248489#c5 and https://bugzilla.mozilla.org/show_bug.cgi?id=1248489#c6 for some of the things I found). As soon as we started fixing it, it broke apart.
But it was a positive thing, since it allowed us to discover Places shutdown was just broken from many versions (from when async shutdown was introduced).

So to put this into an history log perspective:
- Places shutdown was moved to async shutdown
- that change broke most of its shutdown consumers, among which sanitize. we didn't notice.
- we started cleaning up the mess in sanitize, not knowing it was completely broken already
- sanitize on startup (when shutdown failed) covered the broken shutdown stuff
- cleaning up the code started uncovering some of the existing bugs
- we found form data, cookies and other stuff was not cleared at all on shutdown, due to being run too late, that made us discover the Places shutdown bug
- fixing shutdown made clear we were taking far more time now on shutdown (bug 1248489)
-> here we are, the patch fixes Places shutdown and sanitization on shutdown

Comment 27

[Marco Bonardo [:mak]]

Timeline: broken from Firefox 41 (bug 1043863)

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8716537 [details]
MozReview Request: Bug 1244650 - Failure to clear Forms and Search Data on exit. r=yoric

Nobody is happy about the size of this patch. But it seems that it is worth taking the risk for the reasons mentioned by Marco
Worth case scenario, we end up in the bad space and we backout it in beta 8

Anyway, taking it.
Should be in 45 beta 7

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like mak landed this on aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/9fc589e11861

Comment 30

[Marco Bonardo [:mak]]

Yes, there is a (further) problem in beta, that is we need to also uplift bug 1211849, or the patch here differs too much from the code in beta... then it would be a nightmare to rebase.
On the positive side, bug 1211849 is code removal...

Comment 31

[Marco Bonardo [:mak]]

I asked for uplift in bug 1211849, I have the rebased patches ready to land. I'm sorry for all the handling here, but time was and keep being limited, doing my best.

Comment 32

[Marco Bonardo [:mak]]

Attachment: BETA patch on top of bug 1211849.

in case I'm not around, this is rebased patch for beta, on top of bug 1211849

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Marco Bonardo [::mak] from comment #31)
> I asked for uplift in bug 1211849, I have the rebased patches ready to land.
> I'm sorry for all the handling here, but time was and keep being limited,
> doing my best.

I approved the uplift request in bug 1211849 on Sylvester's behalf. You can land them m-b now so it's included in 45.0b7 build.

Comment 34

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32608d640a8c

Comment 35

[Robert Kaiser]

(In reply to Marco Bonardo [::mak] from comment #26)
> So to put this into an history log perspective:

Wow. Sounds like overall it was good we found this by digging in, but it's very risky. We surely will need some concentrated QA around this.

Comment 36

[DJ-Leith]

OS Windows 7, 64 bit.

Firefox Developer Edition (en-GB) 46.0a2 (2016-02-19) 32 bit.

Name            Firefox
Version         46.0a2
Build ID 	20160219004008
Update Channel  aurora
User Agent      Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0

Multiprocess Windows 	0/2 (Disabled)

Since updating to 46.0a2 (2016-02-19)

Both 'Firefox Shutdown' and 'Firefox Restart' have been crashing.

I have many (more than 12) Profiles.
All clear cache, cookies etc on closing Firefox.
Here is some information from about:support

privacy.clearOnShutdown.history              false
privacy.clearOnShutdown.offlineApps          true
privacy.clearOnShutdown.siteSettings         true

privacy.sanitize.didShutdownSanitize         true
privacy.sanitize.migrateClearSavedPwdsOnExit true
privacy.sanitize.migrateFx3Prefs             true
privacy.sanitize.sanitizeInProgress
  ["cache","cookies","offlineApps","formdata","downloads","sessions","siteSettings"]
privacy.sanitize.sanitizeOnShutdown          true

All have NoScript (and other Extensions) and all have 'e10s off'.

Using the STR (below) I can no longer 'Close Firefox normally',
nor Restart normally, nor Restart as part of updating an Extension
that requires a Restart.

I have seen this in Aurora (my main browser, 32 bit) and Nightly (x64 on Windows)
since 201-02-19.  Firefox 44.0.2 and ESR (38.6.1) do NOT have this issue.

STR

1. Start Firefox and navigate to this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1244650

2. In Task Manager <Ctrl>+<SHIFT>+<Esc>
Tick "Show processes from all users"

Observe that
firefox.exe *32
C:\Program Files (x86)\Aurora\firefox.exe
"FirefoxDeveloperEdition"

and
FlashPlayerPlugin_20_0_0_306.exe *32
C:\Windows\SysWOW64\Macromed\Flash\FlashPlayerPlugin_20_0_306
"Adobe Flash Player 20.0 r0"
are running.
There are TWO FlashPlayerPlugins, with different PIDs.

All of this is 'normal expected behaviour'.

3. In Firefox, open a New Tab and type about:support

4. In the "Crash Reports for the Last 3 Days" section, clear all Crash Reports (if any).

5. use the 'back button in this section' to return to "about:support" and then
click the "Restart with Add-ons Disabled" button.

Expected Result:
Aurora should Restart and on Restarting have two Tabs, this bug and about:support

  You should see two Warnings about "Safe Mode", one before Restarting and one as
  you are Restarting.

Actual Result:
Aurora crashes.

5.1 You do see the first warning, about Safe Mode BEFORE the Restart (expected).
5.2 Then 'the browser window is not visible'.
5.3 About 65 seconds later, in Task Manager, firefox.exe closes
5.4 At this point a "Plugin Container for FirefoxDeveloperEdition has stopped working"
(both FlashPlayerPlugins are still running).

5.4.1
Problem signature:
  Problem Event Name:	APPCRASH
  Application Name:	plugin-container.exe
  Application Version:	46.0.0.5893
  Application Timestamp:	56c7022b
  Fault Module Name:	mozglue.dll
  Fault Module Version:	46.0.0.5893
  Fault Module Timestamp:	56c6f2ce
  Exception Code:	80000003
  Exception Offset:	0000f28b
  OS Version:	6.1.7601.2.1.0.768.3
  Locale ID:	2057
  Additional Information 1:	0a9e
  Additional Information 2:	0a9e372d3b4ad19135b953a78882e789
  Additional Information 3:	0a9e
  Additional Information 4:	0a9e372d3b4ad19135b953a78882e789

5.5 click "Close the program".
At this point, both FlashPlayerPlugins stop running.

5.6 The Mozilla Crash Reporter is also on the Windows Task Bar.
I have sent a Report
bp-890f7cab-20e5-48bf-8753-837e72160220
(most times I did not).
I chose "Restart Firefox".
It did restart with Addons disabled (but just one empty Tab)
and 'Maximised' (Aurora was not Maximised at 1, 3, 4 and 5). 

5.7 When I close Aurora again, after the Safe Mode, I see 5.2 to 5.6 again.

DJ-Leith

continued ...

Comment 37

[DJ-Leith]

continued

OS Windows 7, 64 bit.

Nightly (x64 for Windows en-GB), is similar to Aurora.
Restarts and Shutdown of Nightly also stopped on 2016-02-19.

Name            Firefox
Version         47.0a1
Build ID        20160219030248
Update Channel  nightly
User Agent      Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
OS              Windows_NT 6.1 x86-64

Same STR as in comment # 36

Difference is there is no Flash Plugin (see 2 - above)

2. In Task Manager <Ctrl>+<SHIFT>+<Esc>
Tick "Show processes from all users"

Observe that
firefox.exe
C:\Program Files\Nightly\firefox.exe
"Nightly"
is running (and no Flash Plugin).

I did NOT do
"4. In the "Crash Reports for the Last 3 Days" section, clear all Crash Reports (if any)."
I kept the Crash Reports.


There is no 5.4 (because there is no Flash Plugin).

5.6 At about 65 seconds
The Mozilla Crash Reporter is also on the Windows Task Bar
and C:\Program Files\Nightly\firefox.exe has stopped.

I have sent a Report
bp-14220840-d93e-4ad5-8235-712e12160220
(this Crash Report was after a Restart following updating an Extension,
using the 'Add-on Manager UI' - normal for 'non-restartless Extensions').

DJ-Leith

Comment 38

[Brian Mastenbrook]

I've been having Firefox hang on shutdown and eventually die at the 60s timeout since this change landed on aurora. This only happens when "Form & Search Data" is selected in the settings for clearing history, and "Remember search and form history" is disabled. This is because FormHistory.update does nothing at all when FormHistory.enabled is false, so neither of the callbacks is ever called and the promise never resolves. Checking whether FormHistory is enabled before yielding the promise in line 412 of sanitize.js fixes the issue for me.

As an aside, I don't really know modern JS, but the lack of a resolve in the handleError callback to FormHistory.update seems sketchy to me. Shouldn't it resolve in either case, since seenException is being used to determine whether an error occurred?

Comment 39

[Brian Mastenbrook]

On second thought I'm pretty sure the "fix" I suggested is incorrect. Turning off search and form history does not appear to immediately clear that history, so a user who (1) realizes they accidentally had it enabled (2) turns it off (3) explicitly clears the history would not actually have that history cleared. Either turning off search and form history should have the effect of immediately clearing the history, or FormHistory should allow history items to be removed even when it is not enabled.

Comment 40

[Marco Bonardo [:mak]]

(In reply to Brian Mastenbrook from comment #38)
> I've been having Firefox hang on shutdown and eventually die at the 60s
> timeout since this change landed on aurora. This only happens when "Form &
> Search Data" is selected in the settings for clearing history, and "Remember
> search and form history" is disabled. This is because FormHistory.update
> does nothing at all when FormHistory.enabled is false, so neither of the
> callbacks is ever called and the promise never resolves.

Thanks, this is very useful and formhistory is very wrong when not invoking the callback. I just found bug 976940.

Comment 41

[Marco Bonardo [:mak]]

Everyone crashing, please refer to bug 1248489 from now on, work is tracked there.

Comment 42

[Bogdan Maris, Desktop QA]

We focused our testing on Clear History during Firefox 45 beta 7-8 Regression testing. 
We reproduced the initial issue intermittently using old Developer Edition 46.0a2 from 2016-01-31 but we did not reproduced it using Firefox 45 beta 7 and 8, so I will go ahead and mark this as verified fixed.

Comment 43

[Bogdan Maris, Desktop QA]

Forgot to mention that we tested across platforms (Windows 7 64-bit, Windows XP 32-bit, Mac OS X 10.8, Ubuntu 14.04 32-bit).

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The landing of this patch seems to have caused bug 1248489 to get *much* worse (to the point of being by far the top crash in nightly, and being unable to ship beta releases with that crash level).

Comment 46

[Marco Bonardo [:mak]]

backed out of Firefox 45 in bug 1248489 along with all the other recent code changes in sanitize, to resolve the crash spike in recent betas.
https://hg.mozilla.org/releases/mozilla-beta/rev/d071c2a3c872

Comment 47

[DJ-Leith]

(In reply to Marco Bonardo [::mak] from comment #41)
> Everyone crashing, please refer to bug 1248489 from now on, work is tracked there.

Bug 1248489 is also closed so I'm going to report some GOOD NEWS here:

In https://bugzilla.mozilla.org/show_bug.cgi?id=1248489#c25
Marco Bonardo [::mak] (on 2016-02-22 at 15:56:21 PST) said:

> yep, to fix this we need at least:
> bug 1249786 - since currently we are sanitizing at every startup, even when not needed
> bug 976940 - users with disabled form history crash every time
> bug 871908 - this is the less trivial fix for which I'm trying to find a solution that
> is small enough to be uplifted.


In https://bugzilla.mozilla.org/show_bug.cgi?id=1248489#c30
Marco Bonardo [::mak] (on 2016-02-23 at 01:21:54 PST) said: 

<snip>
> the "disable form history" hang is simple to fix, bug 976940 will land as
> soon as the tree reopens. But I doubt all the hangs are just due to people
> disabling form history.

Name           Firefox
Version        47.0a1
Build ID       20160224030246
Update Channel nightly
User Agent     Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
OS             Windows_NT 6.1 x86-64

Nightly Win64 (en-GB) 

I think Build ID 20160224030246 has both of these fixed bugs:
bug 976940 "FormHistory.update() should throw if form history is disabled and the operation is not a removal"
and bug 1249786 "Sanitize on startup prefs are broken"

  Just as I post this comment I now see that bug 976940 has been reopened,
  https://bugzilla.mozilla.org/show_bug.cgi?id=976940#c10 and backed out.

My Nightly [Build ID 20160224030246], which lost the ability to Restart or
Shutdown on 2016-02-19 [1], is able to Restart and also Shutdown without crashing.

I hope, if these bugs are uplifted to Aurora (or any better fix e.g. backouts)
that my 46.0a2 Profiles will also regain their ability to Restart and Shutdown
without crashing.

I don't think I have had any data loss: I'm trying to 'discard data'
(including Form History).
In my case, I am NOT clearing Browsing History (which is stored in places.sqlite.

[1] Reported in
https://bugzilla.mozilla.org/show_bug.cgi?id=1244650#c36 [Aurora]
https://bugzilla.mozilla.org/show_bug.cgi?id=1244650#c37 [Nightly] 

DJ-Leith

Comment 48

[Marco Bonardo [:mak]]

yeah sorry I forgot to update the reference bug, the work for Nightly and Aurora is tracked in bug 1250424

I had to backout one part of that work, but should reland today.

Comment 49

[Carsten Book [:Tomcat]]

(In reply to Marco Bonardo [::mak] from comment #48)
> yeah sorry I forgot to update the reference bug, the work for Nightly and
> Aurora is tracked in bug 1250424
> 
> I had to backout one part of that work, but should reland today.

hey mak, for the relanding seems this is still needed and beta/rc time is today ?

Comment 50

[Marco Bonardo [:mak]]

(In reply to Carsten Book [:Tomcat] from comment #49)
> hey mak, for the relanding seems this is still needed and beta/rc time is
> today ?

relanding of what?
This has been backed out, that means this bug won't be fixed in time for 45, it will keep being bogus as it was from at least Firefox 41.

Comment 51

[Marco Bonardo [:mak]]

Ah sorry, comment 48 was about relanding bug 976940 in Nightly.

Comment 52

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
The data have been cleared after restart.

Actual Results: 
As expected.