1273475 - Crash in shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait | nsNSSActivityState::restrictActivityToCurrentThread

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Patrick McManus [:mcmanus]]

just hit https://crash-stats.mozilla.com/report/index/fa3c765d-623c-4d4f-8883-d161e2160517 - on nightly 49

There seem to 100+ of them in crash stats without a bug that I can find. Perhaps low hanging project uptime fruit. It appears to be older than 45

https://crash-stats.mozilla.com/signature/?signature=shutdownhang+%7C+WaitForSingleObjectEx+%7C+WaitForSingleObject+%7C+PR_WaitCondVar+%7C+mozilla%3A%3ACondVar%3A%3AWait+%7C+nsNSSActivityState%3A%3ArestrictActivityToCurrentThread#summary

Comment 1

[:Cykesiopka]

(Updating summary so that hopefully this bug shows up on crash-stats.)

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait | nsNSSActivityState::restrictActivityToCurrentThread':
 - nightly (version 50): 60 crashes from 2016-06-06.
 - aurora  (version 49): 579 crashes from 2016-06-07.
 - beta    (version 48): 5339 crashes from 2016-06-06.
 - release (version 47): 0 crashes from 2016-05-31.
 - esr     (version 45): 0 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       6      12       8       7       6       8       5
 - aurora       72      85      86      71      85      78      66
 - beta        907     767     746     699     713     628     555
 - release       0       0       0       0       0       0       0
 - esr           0       0       0       0       0       0       0

Affected platform: Windows

Comment 4

[Patrick McManus [:mcmanus]]

That looks like it needs attention..

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Well, bad news is we introduced a deadlock. Good news is I'm fairly sure I've identified what it is and that the patch I'm working on fixes it. I'll have that ready tomorrow.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: bug 1273475 - fix deadlock and potential crash when PSM shuts down NSS

This fixes two issues:
1. nsNSSShutDownList::evaporateAllNSSResources could deadlock by acquiring
sListLock and then the singleton's mNSSActivityStateLock in
nsNSSActivityState::restrictActivityToCurrentThread.

2. Calling UnloadLoadableRoots before
nsNSSShutDownList::evaporateAllNSSResources could result in removing modules
that were still in use, causing assertion failures and potential crashes.

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

Comment 7

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8777512 [details]
bug 1273475 - fix deadlock and potential crash when PSM shuts down NSS

https://reviewboard.mozilla.org/r/69044/#review66200

I guess this would uplift into Aurora. Since it won't get a full cycle in Nightly, perhaps this should use MOZ_RELEASE_ASSERT so we collect destructor-wrong-thread crashes in the Beta cycle too, instead of hanging. I'd love to get Tim's feedback on that.

Otherwise, r+, and please carry the r+ through if we're moving to RELEASE_ASSERT.

Comment 8

[J.C. Jones [:jcj] (he/him)]

Patrick - is the 5000 figure for 48 up there in Comment 3 high? Like, should we expect to need to uplift this all the way to firefox48?

Comment 9

[Patrick McManus [:mcmanus]]

(In reply to J.C. Jones [:jcj] from comment #8)
> Patrick - is the 5000 figure for 48 up there in Comment 3 high? Like, should
> we expect to need to uplift this all the way to firefox48?

its certainly possible as a ride along.. sylvestre would be the person I would ask.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Yes, probably. Could you fill the uplift request to Aurora, beta and release?  Thanks

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8777512 [details]
bug 1273475 - fix deadlock and potential crash when PSM shuts down NSS

https://reviewboard.mozilla.org/r/69044/#review66608

LGTM

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to J.C. Jones [:jcj] from comment #7)
> I guess this would uplift into Aurora. Since it won't get a full cycle in
> Nightly, perhaps this should use MOZ_RELEASE_ASSERT so we collect
> destructor-wrong-thread crashes in the Beta cycle too, instead of hanging.
> I'd love to get Tim's feedback on that.

Yeah, that might indeed be a good idea to find misbehaving code earlier.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

The volume is not very big on release. As it is a shutdown issue, I won't take that as a ride along.

Comment 14

[J.C. Jones [:jcj] (he/him)]

OK, having trouble getting my follow-on patch to add-in to Keeler's MozReview (it's opening a whole new review w/ his patch included), so let's get the first patch landed and I'll add the follow-on after.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/990954b29382
fix deadlock and potential crash when PSM shuts down NSS. r=ttaubert, r=jcj

Comment 16

[J.C. Jones [:jcj] (he/him)]

Attachment: bug 1273475 - use release assertions for PSM->NSS shutdown

Update the prior patch to use MOZ_RELEASE_ASSERT so that we cleanly crash
if there's still an error somewhere in this logic, instead of deadlocking
on beta (but crashing on aurora).

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/990954b29382

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8778981 [details]
bug 1273475 - use release assertions for PSM->NSS shutdown

https://reviewboard.mozilla.org/r/70054/#review67618

Comment 19

[J.C. Jones [:jcj] (he/him)]

Marking checkin-needed for the release assertions patch. Leaving it open, still.

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=028ec8bac2c4

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e33d3604f5
use release assertions for PSM->NSS shutdown. r=ttaubert

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/f7e33d3604f5

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8777512 [details]
bug 1273475 - fix deadlock and potential crash when PSM shuts down NSS

Approval Request Comment
[Feature/regressing bug #]: bug 1235634 appears to have made this worse, although I think this issue has been present for a long time
[User impact if declined]: small potential for a shutdown crash and/or hang
[Describe test coverage new/current, TreeHerder]: has a test
[Risks and why]: low - the worst that will happen is we traded one kind of crash for another kind of crash
[String/UUID change made/needed]: none

Comment 23

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

I don't see any crashes with this signature for the last week on beta. I checked back to 2016-08-01 and still don't see anything (on any channel). 

David, do you still think we need to uplift this? Maybe some other uplift shifted the crash signature. I do remember this being a high volume signature a while back.

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I think users are still hitting this fairly often:

https://crash-stats.mozilla.com/signature/?signature=shutdownhang%20%7C%20mozilla%3A%3ACondVar%3A%3AWait%20%7C%20nsNSSActivityState%3A%3ArestrictActivityToCurrentThread&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports

(looks like the signature changed a bit, but those stacks looks like exactly what this patch is intended to fix)

Comment 25

[Marco Castelluccio [:marco]]

Yes, looks like there are a lot of shutdown hangs where nsNSSActivityState::restrictActivityToCurrentThread is on the stack: https://crash-stats.mozilla.com/search/?product=Firefox&proto_signature=~nsNSSActivityState%3A%3ArestrictActivityToCurrentThread&_sort=-date&_facets=signature&_facets=version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature.

Comment 26

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

Comment on attachment 8777512 [details]
bug 1273475 - fix deadlock and potential crash when PSM shuts down NSS

Let's uplift this, as we are still getting a few hundred crashes per beta release per week. It should land for beta 8. 
Thanks David and Marco!

Comment 27

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

This hit conflicts for me uplifting it to aurora, so it likely doesn't apply to beta. Could we get rebased patches for this?

Comment 28

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I applied the following:

https://hg.mozilla.org/mozilla-central/rev/990954b29382
https://hg.mozilla.org/mozilla-central/rev/f7e33d3604f5

on top of https://hg.mozilla.org/releases/mozilla-aurora/rev/285149724c55 without conflicts. Is that the right base? What were the conflicts you encountered?

In terms of beta, there definitely are conflicts. I had forgotten that bug 1284946 hasn't gotten as far as beta yet. I'll investigate making a branch patch for that (and if it will even be a good idea to do so).

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/97cd2fba07b3
https://hg.mozilla.org/releases/mozilla-aurora/rev/d52d39c46763

Comment 30

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch for beta (1 of 2)

Comment 31

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch for beta (2 of 2)

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Ok - the beta conflicts were nowhere near as considerable as I thought they were going to be. I think we're fine to move forward with uplifting this to beta.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2406bffc6370
https://hg.mozilla.org/releases/mozilla-beta/rev/e2d17f2657b8