Investigate sanitize.js shutdown hangs/crashes

NEW
Assigned to

Status

()

Core
General
2 years ago
2 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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.
See Also: → bug 1243549
Assignee: nobody → dteller
So, it's pretty clear that nsICookieManager doesn't shutdown properly. That's one of the causes of the issue.
Created attachment 8715376 [details]
shutdown.txt

Best guess: it's related to these errors.
Depends on: 1245578
My bet is that bug 1245578 will fix a big subset of these crashes.
Created attachment 8716269 [details]
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/
Attachment #8716269 - Flags: review?(mak77)
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 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+
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 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

2 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.
Blocks: 1241986
(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
Blocks: 1245101
You need to log in before you can comment on or make changes to this bug.