Closed Bug 1253204 Opened 8 years ago Closed 8 years ago

Don't abort shutdown on plugins sanitization

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox45 --- disabled
firefox46 + fixed
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: mak, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1253204 - Don't abort shutdown on plugins sanitization;r?mak
58 bytes, text/x-review-board-request
mak
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details 
We can't do much about plugins sanitization taking huge time in a short time.

So, alternatively, we can try to avoid waiting too much for it, in the end we don't get immediate benefits from knowing a plugin causes us to crash on shutdown, since we can't control their code.

We can either set a timeout after which we give up waiting for the plugin, or we could just not wait at all. In any case we should try to retain the plugin sanitization telemetry.
David, thoughts?
Flags: needinfo?(dteller)
Mostly, do we have any advantage in a soft timeout vs not waiting at all?
Well, soft timeout means "ok, let's wait at least 10 seconds", while not waiting at all means, if other sanitizations are fast "don't wait at all". I'd go for the soft timeout.
Flags: needinfo?(dteller)
I'll try and get this done now-ish, if we agree.
Sure, the only thing I care about is that FX_SANITIZE_PLUGINS keeps accounting for the whole time, not the timeout.
above the "workaround" please add a TODO (bug 1249333) so if we get any final solution for that we can remove the soft timeout.
Would the soft-timeout only affect plugin sanitization during shutdown or also when selecting the appropriate menu entry while Firefox keeps running.
whimboo: It would just be less likely to crash during shutdown.
I'm asking because of bug 1241986, which shows us intermittent failures when sanitizing cookies. And as it looks like it exists because of the Flash plugin.
While investigating shutdown hangs/crashes, we determined that one of
the sources was plugin cookie sanitization, which is sometimes
insanely long. As a workaround, and until we have solved this in bug
1249333, this patch inroduces soft timeouts that will not hang/crash
Firefox until sanitization of plugin cookies is complete.

Review commit: https://reviewboard.mozilla.org/r/37971/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37971/
Attachment #8726396 - Flags: review?(mak77)
(In reply to Henrik Skupin (:whimboo) from comment #9)
> I'm asking because of bug 1241986, which shows us intermittent failures when
> sanitizing cookies. And as it looks like it exists because of the Flash
> plugin.

It won't change a thing, we are just trying to not abort on purpose, showing the crash dialog to the user, if the problem is a plugin.
Comment on attachment 8726396 [details]
MozReview Request: Bug 1253204 - Don't abort shutdown on plugins sanitization;r?mak

https://reviewboard.mozilla.org/r/37971/#review34649

I'm sorry, imo this is too complex and abstract, it makes the cookies sanitization hard to read with only advantages for the plugins section.
Also considered this should be uplifted, I'm not comfortable with the change.

May we make something that only touches the plugins section of the cookies sanitizer? it shouldn't be hard, you can factor out only the plugins part.
Attachment #8726396 - Flags: review?(mak77)
Tracking this for 46+ since we had problems with these types of shutdown crashes on 45 and this is part of the cleanup effort.
status-firefox45: --- → disabled
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Comment on attachment 8726737 [details]
MozReview Request: Bug 1253204 - Don't abort shutdown on plugins sanitization;r?mak

https://reviewboard.mozilla.org/r/38217/#review35025
Attachment #8726737 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/0fcaa28d5194
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8726737 [details]
MozReview Request: Bug 1253204 - Don't abort shutdown on plugins sanitization;r?mak

Approval Request Comment
[Feature/regressing bug #]: no bug
[User impact if declined]: Crash on shutdown if a plugin takes many seconds to clear its data
[Describe test coverage new/current, TreeHerder]: nightly
[Risks and why]: the change is as self contained as possible, so the risk is limited to plugins clearing. We can track progress through crash-stats
[String/UUID change made/needed]: none
Attachment #8726737 - Flags: approval-mozilla-beta?
Attachment #8726737 - Flags: approval-mozilla-aurora?
Comment on attachment 8726737 [details]
MozReview Request: Bug 1253204 - Don't abort shutdown on plugins sanitization;r?mak

This will help improve shutdown hang situation, taking it.
Attachment #8726737 - Flags: approval-mozilla-beta?
Attachment #8726737 - Flags: approval-mozilla-beta+
Attachment #8726737 - Flags: approval-mozilla-aurora?
Attachment #8726737 - Flags: approval-mozilla-aurora+
Hi Kairo, Philipp, this is just an fyi in case we notice things getting worse instead of improving once this lands in 47 and 46.
Flags: needinfo?(madperson)
Flags: needinfo?(kairo)
Thanks, I have been following this bug already. ;-)
Flags: needinfo?(kairo)
(In reply to Ritu Kothari (:ritu) from comment #20)
> Hi Kairo, Philipp, this is just an fyi in case we notice things getting
> worse instead of improving once this lands in 47 and 46.

nothing obviously bad happened so far :))
Flags: needinfo?(madperson)
(In reply to philipp from comment #23)
> (In reply to Ritu Kothari (:ritu) from comment #20)
> > Hi Kairo, Philipp, this is just an fyi in case we notice things getting
> > worse instead of improving once this lands in 47 and 46.
> 
> nothing obviously bad happened so far :))

That's great! Thank you for the follow up.
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

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: 
Developer specific testing

Actual Results: 
As expected
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: