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

This suggests to implement a PSM-specific workaround for bug 730495, which is required for bug 783994.

(The workaround makes sense for now, because solving bug 730495 in general seems difficult and has unknown timing.)

As mentioned in bug 730495, the workaround is to require loading of mozStorage from within PSM module init, prior to calling NSS library init.

Once bug 730495 is fixed in a general way, this workaround could get removed.

The patch has already been reviewed in bug 730495 comment 34.

I'm attaching a second revision of the patch, which contains the changes that David had requested.

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

(In reply to David Keeler [:keeler] (use needinfo?) from bug 730495 comment #34)
> I think it might be best to put this in nsNSSComponent::Init so that it's
> impossible to instantiate PSM without the storage service. Also, since we're
> not checking rv, I think we can use the do_GetService variant that doesn't
> have the extra parameter.

New patch attached. Does the error code make sense?

try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa54693e2e6920cc8ee328085ce617494f3a3e7

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 2 • 7 years ago

Comment on attachment 8886235 [details] [diff] [review]
1380706-v2.patch

Review of attachment 8886235 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Just the one nit and I think we should add a comment. Thanks!

::: security/manager/ssl/nsNSSComponent.cpp
@@ +56,5 @@
>  #include "sslerr.h"
>  #include "sslproto.h"
>  #include "prmem.h"
>  
> +#include "mozStorageCID.h"

nit: this should be earlier in the include list (it's intended to be sorted alphabetically, case-sensitive)

@@ +2038,5 @@
>  
> +  nsCOMPtr<nsISupports> storageService =
> +    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
> +  if (!storageService) {
> +    return NS_ERROR_NOT_AVAILABLE;

Yep - I think this is a good error code. Let's also have a comment explaining why we added this.

	Pulsebot
	Comment 3 • 7 years ago

Pushed by kaie@kuix.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e722cacf61
PSM should depend on mozStorage, as a workaround for a sqlite3_config race, r=keeler

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

Thanks, pushed including the requested changes.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 5 • 7 years ago
bugherder

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

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

for the record, this workaround was removed, as bug 730495 has been fixed.

	Jan Beich
	Comment 7 • 7 years ago

Can you backport this to ESR52 to fix a crash --with-system-nss after bug 1377940?

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225364

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

Jan, several fixes were applied to Firefox to make it fully with the NSS SQL DB, this bug is just one of the related bugs. I don't think we can backport all of them. If you're using NSS 3.35+ as a shared system library, you could consider to patch NSS to keep the old DBM default, as long as you cannot yet upgrade to Firefox 58 or newer, and plan to accept the change of default in NSS at the same time when you will upgrade your environment to Firefox 58+.

In any way, this bug is probably not the right place for this discussion, I suggest the mozilla.dev.tech.crypto list.

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

s/fully/fully compatible/

	Mark Straver
	Comment 11 • 6 years ago

> several fixes were applied to Firefox to make it fully [compatible] with the NSS SQL DB, this bug is just one of the related bugs.

Is there a record of the applicable bugs for these "several fixes"? Backporting may be some work but for some projects hopping onto Quantum is simply a no-go area.

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

Mark, you can find the relevant work, by looking at bug 783994 and bug 1377940, and for each of them, by looking at their "depends on" lists.

	Mark Straver
	Comment 13 • 6 years ago

Thanks Kal,

I've made note of it, if we have to revisit this later on (i.e. if we're forced to move to SQL which I hope won't be needed).

Our current solution is to prefix the cert db path with 'dbm:' to tell NSS we prefer DBM so we avoid this race condition and can keep using our preferred db format.