696551 - Make nsSyncJPAKE handle NSS shutdown/restart properly

Status: RESOLVED DUPLICATE of bug 1233853

(Firefox :: Sync, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

nsSyncJPAKE doesn't implement nsNSSShutdownObject and the related protocol for dealing with PSM/NSS shutdown. This can lead to bad things happening during private browsing mode transitions and/or shutdown or profile change.

Comment 1

[Richard Newman [:rnewman]]

Re Bug 702307 crasher: how confident are you in this hunch, bsmith, and are you available for review?

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am not confident in it at all. In fact, it would seem a little unlikely because it would mean all those crashes were happening while people were trying to set up Sync. that seems very unlikely. (We only use nsSyncJPAKE during Sync sign-up, right?)

I can do a review of changes to nsSyncJPAKE that make it return "NSS is shut down" errors to the Sync code that uses JPAKE. I probably can't review changes to how the Sync code reacts to that.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Brian Smith (:bsmith) from comment #2)
> I am not confident in it at all. In fact, it would seem a little unlikely
> because it would mean all those crashes were happening while people were
> trying to set up Sync. that seems very unlikely. (We only use nsSyncJPAKE
> during Sync sign-up, right?)

Correct. We don't kill refs to nsSyncJPAKE, but we do kill refs to the the JPAKEClient object in Sync, which should be the only thing holding on to it.

WeaveCrypto doesn't appear to implement any shutdown logic, but holds on to ctypes references to NSS functions. Is that another lead?

> I can do a review of changes to nsSyncJPAKE that make it return "NSS is shut
> down" errors to the Sync code that uses JPAKE. I probably can't review
> changes to how the Sync code reacts to that.

OK. I'm going to dial down the prio for this for now unless we can get a clear smoking gun for a crash bug.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Richard Newman [:rnewman] from comment #3)
> WeaveCrypto doesn't appear to implement any shutdown logic, but holds on to
> ctypes references to NSS functions. Is that another lead?

Yes, let's file another bug for that. Basically, the rule is that before you use any NSS function, you must:
1. Acquire the NSS shutdown prevention lock
2. check that NSS hasn't been shut down yet
3. do_GetService(nsINSSComponent) and ensure that it returns a non-NULL reference, and hold that reference (even if you don't use anything from that interface).
4. make your NSS calls.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Brian Smith (:bsmith) from comment #4)

> Yes, let's file another bug for that.

Filed Bug 702519.

Comment 6

[Allison Naaktgeboren :ally]

sync triage: hey brian, how's this going?