1253204 - Don't abort shutdown on plugins sanitization

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

David, thoughts?

Comment 2

[Marco Bonardo [:mak]]

Mostly, do we have any advantage in a soft timeout vs not waiting at all?

Comment 3

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

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.

Comment 4

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

I'll try and get this done now-ish, if we agree.

Comment 5

[Marco Bonardo [:mak]]

Sure, the only thing I care about is that FX_SANITIZE_PLUGINS keeps accounting for the whole time, not the timeout.

Comment 6

[Marco Bonardo [:mak]]

above the "workaround" please add a TODO (bug 1249333) so if we get any final solution for that we can remove the soft timeout.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Would the soft-timeout only affect plugin sanitization during shutdown or also when selecting the appropriate menu entry while Firefox keeps running.

Comment 8

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

whimboo: It would just be less likely to crash during shutdown.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 10

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

Attachment: MozReview Request: Bug 1253204 - Don't abort shutdown on plugins sanitization;r?mak

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/

Comment 11

[Marco Bonardo [:mak]]

(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 12

[Marco Bonardo [:mak]]

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.

Comment 13

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

Attachment: MozReview Request: Bug 1253204 - Don't abort shutdown on plugins sanitization;r?mak

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this for 46+ since we had problems with these types of shutdown crashes on 45 and this is part of the cleanup effort.

Comment 15

[Marco Bonardo [:mak]]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fcaa28d5194

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/0fcaa28d5194

Comment 18

[Marco Bonardo [:mak]]

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

Comment 19

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

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.

Comment 20

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

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.

Comment 21

[Robert Kaiser]

Thanks, I have been following this bug already. ;-)

Comment 22

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45c68ee48929
https://hg.mozilla.org/releases/mozilla-beta/rev/654c0f3c051f

Comment 23

[[:philipp]]

(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 :))

Comment 24

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

(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.

Comment 25

[Mayur Patil]

[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