1245065 - Investigate sanitize.js shutdown hangs/crashes

Status: NEW

(Core :: General, defect)

Description

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

According to AWSDY ( http://yoric.github.io/are-we-shutting-down-yet-/?version=Firefox+47.0a1&version=Firefox+46.0a2&version=Firefox+45.0b1&version=Firefox+45.0a2&version=Firefox+44.0b99&version=Firefox+44.0b9&version=Firefox+44.0b1&version=Firefox+44.0&version=Firefox+43.0b3 ), we have many shutdown hangs/crashes in sanitization.

Some of these hangs/crashes take place while clearing formdata, some while clearing cookies.

Given how sanitization works, there is the possibility that we are attempting to clear stuff after windows and other stuff are uninitialized, which could explain hangs.

Comment 1

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

Interestingly, we seem to have 0 signatures of this crash in FF46 and almost none in FF47 (only ~70 crashes on the last 7 days):

http://yoric.github.io/are-we-shutting-down-yet-/?version=Firefox+47.0a1&signature=%7Esanitize.js%3A+Sanitize&signature=%7Esanitize.js%3A+Sanitize+on+shutdown#
http://yoric.github.io/are-we-shutting-down-yet-/?version=Firefox+47.0a1&signature=%7Esanitize.js%3A+Sanitize#

Comment 2

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

So, it's pretty clear that nsICookieManager doesn't shutdown properly. That's one of the causes of the issue.

Comment 3

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

Attachment: shutdown.txt

Best guess: it's related to these errors.

Comment 4

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

My bet is that bug 1245578 will fix a big subset of these crashes.

Comment 5

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

Attachment: MozReview Request: Bug 1245065 - Making sanitize.js handle properly nsCookieService shutdown;r?mak

These days, the sanitizer often attempts to cleanup cookies during
shutdown *after* the cookie service has been disconnected, hence when
it is too late. This causes privacy issues and is suspected of causing
shutdown hangs/crashes.

Bug 1245578 introduces a shutdownClient for the nsCookieService. In
this patch, we make use of this shutdownClient to properly handle
sanitization of the cookies during shutdown.

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

Comment 6

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

The attached patch (which is based on bug 1245578) should fix the sanitization of cookies during shutdown.

We still need to fix "sessions". I'm not sure what that is yet.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8716269 [details]
MozReview Request: Bug 1245065 - Making sanitize.js handle properly nsCookieService shutdown;r?mak

https://reviewboard.mozilla.org/r/33769/#review30449

::: browser/base/content/sanitize.js:704
(Diff revision 1)
> -     .jsclient;
> +      .jsclient;

one uses Cc, the other one Components.classes, and dots should be aligned with [

Comment 8

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

Comment on attachment 8716269 [details]
MozReview Request: Bug 1245065 - Making sanitize.js handle properly nsCookieService shutdown;r?mak

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

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8716269 [details]
MozReview Request: Bug 1245065 - Making sanitize.js handle properly nsCookieService shutdown;r?mak

https://reviewboard.mozilla.org/r/33769/#review30521

::: browser/base/content/sanitize.js:716
(Diff revision 2)
> +    return deferredSanitization.promise;

I'd prefer if you'd unified the return code by inverting the if. It would be less error-prone to future changes.

Comment 10

[quality+bugzilla]

Will we have a 44.0.2 release for this and 1245578 (and possibly 1243549)?  Without these repaired, sanitize on exit is not reliable, which is a significant privacy issue.

This is a key issue as defined by Principle #4 of The Mozilla Manifesto (https://www.mozilla.org/about/manifesto/).

Accordingly, I recommend an increase in importance.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #9)
> I'd prefer if you'd unified the return code by inverting the if. It would be
> less error-prone to future changes.

I'm fixing this directly in bug 1243549

Comment 12

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

Not working on this anymore.