Kai Engert (:KaiE:) Assignee
	Description • 7 years ago

When working on bug 783994, the following xpcshell test:
  services/fxaccounts/tests/xpcshell/test_push_service.js

fails with this assertion:

"xpcshell: /pd/moz/mocent/mozilla/db/sqlite3/src/sqlite3.c:23369: sqlite3MutexAlloc: Assertion `GLOBAL(int, mutexIsInit)' failed."

This could potentially be a duplicate of bug 730495, which currently doesn't have a general fix in sight.

Maybe we could implement a similar workaround as used in bug 1380706.

	Kai Engert (:KaiE:) Assignee
	Comment 1 • 7 years ago

This workaround is equivalent to what we did in 1380706, and it fixes the test.

(I haven't investigated why PSM implicitly dependencing on the storage service was insufficient.)

Justin, I saw you had added "this._crypto" in the past (bug 717490), to explicitly depend on PSM. Could you please review/confirm that this additional workaround is acceptable, too? Thanks.

	Kai Engert (:KaiE:) Assignee
	Comment 2 • 7 years ago

While working on this, I discovered bug 1401590.

	Kai Engert (:KaiE:) Assignee
	Comment 3 • 7 years ago

For reference:

The original failure could be seen in this try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e79fd912831869272317d4363156ce253100a3b
Linux: tc-X(X7), mac/windows: tc-X(X)

These try build contain the fix attached to this bug:
Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d38c24961996f40da8514d5ea1b02d6f6d693c97
win/mac: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee175d0147ff6c86488b91e8bdb17e82c7f32b3b

The attached patch works to get us back to the previous state (no assertion), 
but doesn't fix bug 1401590 (separate, pre-existing issue).

	Justin Dolske [:Dolske]
	Comment 4 • 7 years ago

Comment on attachment 8910288 [details] [diff] [review]
1401514-v1.patch

If we do this workaround, it would be better to have it live in crypto-SDR.js's init() [right before we touch nsIPK11TokenDB, which is presumably what's causing PSM to initialize in problematic cases, and already has a bit of a workaround for making sure the softtoken is in a good state].

I suppose I could live with that, but I'm not sure that's really the right fix... Can't you do this down in SecretDecoderRing.cpp itself, as with bug 1380706? [Or, more generally, any other entry point PSM entry point that has the possibility of invoking NSS before Storage?]

That would also have the benefit of having this workaround live closer to the code that actually needs it.

And just to be clear, it sounds like this is a problem that only happens with your work in bug 783994 (and so isn't yet affecting Firefox users)?

	Kai Engert (:KaiE:) Assignee
	Comment 5 • 7 years ago

(In reply to Justin Dolske [:Dolske] from comment #4)
> And just to be clear, it sounds like this is a problem that only happens
> with your work in bug 783994 (and so isn't yet affecting Firefox users)?

Correct, not yet.

	Kai Engert (:KaiE:) Assignee
	Comment 6 • 7 years ago

(In reply to Justin Dolske [:Dolske] from comment #4)
> Can't you do this down in SecretDecoderRing.cpp itself, as with bug
> 1380706? [Or, more generally, any other entry point PSM entry point that has
> the possibility of invoking NSS before Storage?]
> 
> That would also have the benefit of having this workaround live closer to
> the code that actually needs it.

Many services offered by PSM require that NSS is already initialized.

Several years ago, PSM had special XPCOM macros, that would automatically trigger the NSS init, regardless which PSM service is requested.

Looking at PSM today, it seems that code was removed. Construction of many PSM services doesn't trigger the init, but simply checks if NSS init was completed already, and returns a failure if it wasn't yet (EnsureNSSInitializedChromeOrContent).

David, do you know why the automatic trigger was removed, or am I misunderstanding? What is today's expectation? Is there a requirement that the nsNSSComponent gets explicitly instantiated, before PSM services may be used? Are you doing that somewhere in the startup code? Do you understand why that code isn't reached in this test?

Do you think the SecretDecoderRing code (and potentially other PSM services) should be enhanced to automatically trigger nsNSSComponent init?

	Kai Engert (:KaiE:) Assignee
	Comment 7 • 7 years ago

(In reply to Kai Engert (:kaie:) from comment #6)
> 
> Several years ago, PSM had special XPCOM macros, that would automatically
> trigger the NSS init, regardless which PSM service is requested.
> 
> Looking at PSM today, it seems that code was removed.

I take this back, sorry. The code is still there, EnsureNSSInitializedChromeOrContent triggers init of the PSM component.
(I had been misguided because of bug 1401590, I had incorrectly concluded that we're failing to init sqlite.)

The assertion
  sqlite3MutexAlloc: Assertion `GLOBAL(int, mutexIsInit)
fails, because of the shutdown order of XPCom components. 

The storage service is shut down first, which calls sqlite3_shutdown().

The PSM service is called later, which calls NSS shutdown, which attempts to close the NSS sqlite databases. That fails, because sqlite has already been shut down.

The attached workaround causes the storage service to get get initialized earlier, and storage/sqlite to be shutdown later than PSM/NSS, which works.

I will file a general bug for this issue, similar to bug 730495, but to ensure that sqlite_shutdown is called later than any other calls to sqlite. That might be difficult to get done, similar as we haven't been able to find a general solution to bug 730495 yet.

	Kai Engert (:KaiE:) Assignee
	Comment 8 • 7 years ago

We've fixed bug 730495, in a way that addresses both init and shutdown.