Open
Bug 1245065
Opened 9 years ago
Updated 2 years ago
Investigate sanitize.js shutdown hangs/crashes
Categories
(Core :: General, defect)
Core
General
Tracking
()
NEW
People
(Reporter: Yoric, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
Attachments
(2 files)
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.
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Comment 1•9 years ago
|
||
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#
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → dteller
Reporter | ||
Comment 2•9 years ago
|
||
So, it's pretty clear that nsICookieManager doesn't shutdown properly. That's one of the causes of the issue.
Reporter | ||
Comment 3•9 years ago
|
||
Best guess: it's related to these errors.
Reporter | ||
Comment 4•9 years ago
|
||
My bet is that bug 1245578 will fix a big subset of these crashes.
Reporter | ||
Comment 5•9 years ago
|
||
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/
Attachment #8716269 -
Flags: review?(mak77)
Reporter | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
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 [
Attachment #8716269 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8716269 -
Flags: review+
Comment 10•9 years ago
|
||
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•9 years ago
|
||
(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
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•