1301407 - Crash in nsNSSComponent::nsNSSComponent

Status: RESOLVED DUPLICATE of bug 1339267

(Core :: Networking, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-3d2b8867-3bba-488b-ae04-5ae892160908.
=============================================================

Seen while looking at Fennec beta 11 crashes: http://bit.ly/2cwb43x

New signature that is currently low volume in RC but didn't appear in released versions. Also present in very small volume on Aurora and Nightly.

Not sure if this should be bucketed in the NSS component.

Comment 1

[Franziskus Kiefer [:franziskus]]

This is coming from bug 1273475.

Comment 2

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

It looks like we're doing a speculative connect before PSM/NSS has been initialized on the main thread:

> 16 libxul.so mozilla::net::nsHttpConnectionMgr::OnMsgSpeculativeConnect netwerk/protocol/http/nsHttpConnectionMgr.cpp:2923

I tried to figure out how this could be happening, but all the code paths I'm finding have already initialized PSM/NSS on the main thread. Maybe Patrick has an idea?

Comment 3

[Patrick McManus [:mcmanus]]

I don't really know what path, but you can probably do this and I'd r+ it

   if (!IsNeckoChild()) {
        // HACK: make sure PSM gets initialized on the main thread.
        net_EnsurePSMInit();
    }
in

nsHttpConnectionMgr::SpeculativeConnect() (in nsHttpConnectionMgr.cpp)

Comment 4

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: bug 1301407 - ensure PSM is initialized on the main thread before doing a speculative connect

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

Comment 5

[[:philipp]]

[Tracking Requested - why for this release]:
this is making up 1.7% of crashes on android release right now and almost all of them crashes on startup.

@patrick - could you also explicitly r+ this change?

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8793963 [details]
bug 1301407 - ensure PSM is initialized on the main thread before doing a speculative connect

https://reviewboard.mozilla.org/r/80544/#review82244

Comment 7

[Patrick McManus [:mcmanus]]

I'm sorry about the review delay. I already reviewed this but I must not have succeeded in publishing it and it fell off the radar. thanks for the ni

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8450da83e7bc
ensure PSM is initialized on the main thread before doing a speculative connect r=mcmanus

Comment 9

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Thanks! (shakes fist at review board...)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8450da83e7bc

Comment 11

[[:philipp]]

can we uplift the fix in terms of scope & risk? (as noted in comment #5, it's a top crasher on android)

Comment 12

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8793963 [details]
bug 1301407 - ensure PSM is initialized on the main thread before doing a speculative connect

Approval Request Comment
[Feature/regressing bug #]: speculative connect causing PSM initialization off the main thread
[User impact if declined]: startup crashes
[Describe test coverage new/current, TreeHerder]: the related features have tests, but nothing covers this specific case
[Risks and why]: low - the fix is small and takes existing behavior and applies it to the new case of speculative connect. Basically the worst that could happen is this doesn't fix the issue.
[String/UUID change made/needed]: none

Comment 13

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

Tracked as it's a top crasher.

Comment 14

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

Comment on attachment 8793963 [details]
bug 1301407 - ensure PSM is initialized on the main thread before doing a speculative connect

Fixes a top crasher on Fennec, Aurora51+, Beta50+

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e11e42465cc

Comment 16

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6b1a3f769aa6

Comment 17

[[:philipp]]

hi, the patch has landed in 50.0b6 but crashes with this signature are continuing:
https://crash-stats.mozilla.com/signature/?product=Firefox&product=FennecAndroid&process_type=browser&version=50.0b6&signature=nsNSSComponent%3A%3AnsNSSComponent#reports

should we reopen the bug?

Comment 18

[Carsten Book [:Tomcat]]

just making sure dkeeler see comment #17

Comment 19

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #12)
...
> Basically the worst that could happen is this doesn't fix the issue.

:(

On a more serious note, I'm sort-of out of ideas as to how this could still be happening.

Comment 20

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

Patrick - I think we/keeler need some necko eyes on this for help in tracking down what could be happening in the stack to even get here. Can you point us to someone who can take a look? Thanks!

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Could we call net_EnsurePSMInit in nsHttpHandler::Init() ? Or is that too early in the startup process?
Is calling GetSocketProvider("ssl") enough, or do we need to keep a ref to the socket provider around?

Comment 22

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

Comment on attachment 8793963 [details]
bug 1301407 - ensure PSM is initialized on the main thread before doing a speculative connect

(Clearing the flags so it doesn't show up on Sheriff queries)

Comment 23

[Dragana Damjanovic [:dragana]]

(In reply to Valentin Gosu [:valentin] from comment #21)
> Could we call net_EnsurePSMInit in nsHttpHandler::Init() ? Or is that too
> early in the startup process?
> Is calling GetSocketProvider("ssl") enough, or do we need to keep a ref to
> the socket provider around?

The question above is more for security people :) I will needinfo them.

we can also do it just after socketTransport thread is created (in nsIOService), that is the earliest :)

Comment 24

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to Valentin Gosu [:valentin] from comment #21)
> Could we call net_EnsurePSMInit in nsHttpHandler::Init() ? Or is that too
> early in the startup process?

It looks like that could work. The only requirements are that it be on the main thread and that, if a profile directory is available, it happens after the directory service is set up to return that directory if queried for it.

> Is calling GetSocketProvider("ssl") enough, or do we need to keep a ref to
> the socket provider around?

My understanding is that since it's a service, it should stay around in the service manager (which keeps a reference to it). Note that GetSocketProvider("ssl") is a bit indirect - you could call do_GetService(PSM_COMPONENT_CONTRACTID) to be more clear that the purpose is to ensure that PSM has properly initialized.

(In reply to Dragana Damjanovic [:dragana] from comment #23)
> we can also do it just after socketTransport thread is created (in
> nsIOService), that is the earliest :)

Again, as long as that's main-thread-only and the profile directory is available, that should work. Earlier is probably better (if we can simplify the reasoning about when PSM/NSS will be available, that would be nice).

Comment 25

[Dragana Damjanovic [:dragana]]

This crash is interesting:

https://crash-stats.mozilla.com/report/index/bfd8eed6-fd15-44ba-830d-e87862161022

It crashes in OnMsgSpeculativeConnect(). The only place we call (correctly we dispatch it to the socketThread) this is in SpeculativeConnect
https://hg.mozilla.org/releases/mozilla-beta/annotate/cefddaa8e227/netwerk/protocol/http/nsHttpConnectionMgr.cpp#l455

A couple of lines above this call we call net_EnsurePSMInit()
https://hg.mozilla.org/releases/mozilla-beta/annotate/cefddaa8e227/netwerk/protocol/http/nsHttpConnectionMgr.cpp#l413

So the only way this can happen is if net_EnsurePSMInit fails somewhere or reference to the service is not kept.

We could check whether functions called by net_EnsurePSMInit have succeeded and if not we should crash. In that way we can get the real reason this fails.

Maybe keeler have an idea in which cases it could fail.

Comment 26

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2c1b51f6025

Comment 27

[Dragana Damjanovic [:dragana]]

Attachment: bug_1301407_ensure_nss_initialized.patch

This will not resolved the bug but just make it fail earlier if PSM cannot be initialized.

try run is in comment 26.

Comment 28

[Dragana Damjanovic [:dragana]]

Attachment: bug_1301407_ensure_nss_initialized.patch

Of course it must be MOZ_RELEASE_ASSERT.

Comment 29

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae45aeb3fb0

Comment 30

[Dragana Damjanovic [:dragana]]

test are failing :)

Comment 31

[Dragana Damjanovic [:dragana]]

Attachment: bug_1301407_ensure_nss_initialized.patch

Comment 32

[Patrick McManus [:mcmanus]]

Comment on attachment 8803869 [details] [diff] [review]
bug_1301407_ensure_nss_initialized.patch

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +414,2 @@
>          // HACK: make sure PSM gets initialized on the main thread.
> +        nsCOMPtr<nsISupports> nss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);

isn't this logic that net_EnsurePSMInit should do?

Comment 33

[Dragana Damjanovic [:dragana]]

 (In reply to Patrick McManus [:mcmanus] from comment #32)
> Comment on attachment 8803869 [details] [diff] [review]
> bug_1301407_ensure_nss_initialized.patch
> 
> Review of attachment 8803869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +414,2 @@
> >          // HACK: make sure PSM gets initialized on the main thread.
> > +        nsCOMPtr<nsISupports> nss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);
> 
> isn't this logic that net_EnsurePSMInit should do?

It does not do it directly but using GetSocketProvider("ssl"). The reason I am doing it this way is to make sure that nssComponent is created, e.g. to assert on NS_SUCCEEEDED(rv).

SpeculativeConnect function calls net_EnsurePSMInit but we still create nssComponent on socketThread and of course fail assertion. (crash from comment 25)

Comment 34

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bb430ed92ff

Comment 35

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8803869 [details] [diff] [review]
bug_1301407_ensure_nss_initialized.patch

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +414,2 @@
>          // HACK: make sure PSM gets initialized on the main thread.
> +        nsCOMPtr<nsISupports> nss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);

Right - maybe we should just fix net_EnsurePSMInit to directly initialize PSM, check for failure, and raise a release assertion if it fails? (Unfortunately, it won't tell us why PSM is failing to initialize...)

Comment 36

[Dragana Damjanovic [:dragana]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #35)
> Comment on attachment 8803869 [details] [diff] [review]
> bug_1301407_ensure_nss_initialized.patch
> 
> Review of attachment 8803869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +414,2 @@
> >          // HACK: make sure PSM gets initialized on the main thread.
> > +        nsCOMPtr<nsISupports> nss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);
> 
> Right - maybe we should just fix net_EnsurePSMInit to directly initialize
> PSM, check for failure, and raise a release assertion if it fails?
> (Unfortunately, it won't tell us why PSM is failing to initialize...)

I know, but I should have been more precise what I want to do.
I want to uplift this to beta, maybe next beta this one is late in the cycle. I was hoping to get a crash stack on the main thread, maybe that will tell us more. This is not a solution, this patch should gave us hopefully more info what is happening. This crash is bad on android.

Comment 37

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66cc17067bdd

Comment 38

[Dragana Damjanovic [:dragana]]

Attachment: bug_1301407_ensure_nss_initialized.patch

Comment 39

[Dragana Damjanovic [:dragana]]

there is still a possibility that nssComponent get destroyed after creation.

Comment 40

[Patrick McManus [:mcmanus]]

Comment on attachment 8804167 [details] [diff] [review]
bug_1301407_ensure_nss_initialized.patch

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

::: netwerk/base/nsNetUtil.cpp
@@ +2126,5 @@
>  
>  void net_EnsurePSMInit()
>  {
> +    nsresult rv;
> +    nsCOMPtr<nsISupports> nss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);

s/nss/psm ?

@@ -2128,5 @@
> -    nsCOMPtr<nsISocketProviderService> spserv =
> -            do_GetService(NS_SOCKETPROVIDERSERVICE_CONTRACTID);
> -    if (spserv) {
> -        nsCOMPtr<nsISocketProvider> provider;
> -        spserv->GetSocketProvider("ssl", getter_AddRefs(provider));

I didn't do the research to make sure this is made redundant by the new code.. just making sure you did :)

Comment 41

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #40)
> Comment on attachment 8804167 [details] [diff] [review]
> bug_1301407_ensure_nss_initialized.patch
> 
> Review of attachment 8804167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsNetUtil.cpp
> @@ +2126,5 @@
> >  
> >  void net_EnsurePSMInit()
> >  {
> > +    nsresult rv;
> > +    nsCOMPtr<nsISupports> nss = do_GetService(PSM_COMPONENT_CONTRACTID, &rv);
> 
> s/nss/psm ?
> 
> @@ -2128,5 @@
> > -    nsCOMPtr<nsISocketProviderService> spserv =
> > -            do_GetService(NS_SOCKETPROVIDERSERVICE_CONTRACTID);
> > -    if (spserv) {
> > -        nsCOMPtr<nsISocketProvider> provider;
> > -        spserv->GetSocketProvider("ssl", getter_AddRefs(provider));
> 
> I didn't do the research to make sure this is made redundant by the new
> code.. just making sure you did :)

Comment 24 states that and I also look through the code a bit.

Comment 42

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8804167 [details] [diff] [review]
bug_1301407_ensure_nss_initialized.patch

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

::: netwerk/base/nsNetUtil.cpp
@@ -2128,5 @@
> -    nsCOMPtr<nsISocketProviderService> spserv =
> -            do_GetService(NS_SOCKETPROVIDERSERVICE_CONTRACTID);
> -    if (spserv) {
> -        nsCOMPtr<nsISocketProvider> provider;
> -        spserv->GetSocketProvider("ssl", getter_AddRefs(provider));

From my reading, the new code really is just a simplification of this original code (the important part was always that it would cause PSM to be initialized).

Comment 43

[Dragana Damjanovic [:dragana]]

Attachment: bug_1301407_ensure_nss_initialized.patch

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14a46bd1ea2
Ensure nss initialized during nsHttpHandler initialization. r=mcmanus, r=keeler

Comment 45

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Too late for a fix in 49. But we still have time to get a fix into 50.

Comment 46

[Phil Ringnalda (:philor)]

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

Comment 47

[Dragana Damjanovic [:dragana]]

I forgot to set "leave-open".
Probably, this patch will not fix the issue. We do not know what goes wrong and this patch should give us more clues.

Comment 48

[Dragana Damjanovic [:dragana]]

I will ask for a uplift to aurora after it runs for a day in nightly.
There is no crashes on nightly but there are some on aurora.

Comment 49

[Dragana Damjanovic [:dragana]]

Comment on attachment 8804360 [details] [diff] [review]
bug_1301407_ensure_nss_initialized.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Startup crash. This patch will not fix the issue, but it could help us by giving us some more clues. Nightly is not crashing therefore we need uplift to aurora (there are some crashes on aurora, very low volume).
[Describe test coverage new/current, TreeHerder]: This code path is very heavily used.
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 50

[Gerry Chang [:gchang]]

Comment on attachment 8804360 [details] [diff] [review]
bug_1301407_ensure_nss_initialized.patch

Take this patch to help investigate the issue. Aurora51+.

Comment 51

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/60538c8091f1

Comment 52

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

Unsure if this means 51 gets marked as fixed, feel free to correct it if I'm wrong.

Comment 53

[Dragana Damjanovic [:dragana]]

(In reply to Wes Kocher (:KWierso) from comment #52)
> Unsure if this means 51 gets marked as fixed, feel free to correct it if I'm
> wrong.

This patch should help us find the problem, most probably it will not fix it. I will change to affected.

Comment 54

[Dragana Damjanovic [:dragana]]

Now we have a bit more clue what is happening.

net_EnsurePSMInit crashes on assert.
For some reason we cannot initialize PSM service.

Comment 55

[Dragana Damjanovic [:dragana]]

any idea ^?

Comment 56

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Well, we could add a release assertion in every place where nsNSSComponent initialization might return a failing error code and see if that tells us which operation is failing. I'll put together a patch.

Comment 57

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: 1301407-nsNSSComponent-asserts.diff

Comment 58

[Dragana Damjanovic [:dragana]]

Comment on attachment 8808374 [details] [diff] [review]
1301407-nsNSSComponent-asserts.diff

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

Thank you for doing this.

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db6a856a9d37
temporarily add assertions during nsNSSComponent::Init to find out what's failing on Android r=dragana

Comment 60

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/788b95bf4ea3
Backed out changeset db6a856a9d37 for Assertion failures at nsNSSComponent.cpp

Comment 61

[Iris Hsiao [:ihsiao]]

Sorry had to back out for Assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38802703&repo=mozilla-inbound#L4854

Comment 62

[Dragana Damjanovic [:dragana]]

Attachment: bug_130147_remove_release_assert.patch

I would remove release assert before this aurora get to beta.

This assert will failed for wrong tls versions(see the back out above), but in that case nsNSSComponent is already created and the original crash will not happened. The original crash was in nsNSSComponent constructor. This assertion will produce even more crashes.

Comment 63

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: 1301407-nsNSSComponent-asserts.diff v2

Looks like some of those asserts were a little over-zealous. This looks better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56451399609e8b58fd63e9d0526278bf63b1543c

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/425f66fa9b10
temporarily add assertions during nsNSSComponent::Init to find out what's failing on Android r=dragana

Comment 65

[Dragana Damjanovic [:dragana]]

Comment on attachment 8808509 [details] [diff] [review]
bug_130147_remove_release_assert.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: I believe that this will cause same assertion crashes. We believed that it was safe to set an assertion at that point in code, but some assertion crashes on Nightly and aurora show that this will happen in cases that we do not want it to happen and that should not crash the browser.
[Describe test coverage new/current, TreeHerder]: It only changes MOZ_RELEASE_ASSERT into MOZ_ASSERT. Current code on beta do not have this assertion, so if we change assertion to MOZ_ASSERT the behavior will be the same as on beta50
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 66

[Dragana Damjanovic [:dragana]]

checkin-needed for bug_130147_remove_release_assert.patch

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/166114662d40
Remove release assert for nss initialization. r=keeler

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/425f66fa9b10

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/166114662d40

Comment 70

[Dragana Damjanovic [:dragana]]

It is crashing at:
1861 rv = LoadExtendedValidationInfo();
1862 #ifdef ANDROID
1863 MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));
1864 #endif
https://hg.mozilla.org/mozilla-central/annotate/336759fad462/security/manager/ssl/nsNSSComponent.cpp#l1863

https://crash-stats.mozilla.com/signature/?version=52.0a1&signature=nsNSSComponent%3A%3AInitializeNSS&date=%3E%3D2016-11-04T09%3A40%3A05.000Z&date=%3C2016-11-11T09%3A40%3A05.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports

Comment 71

[Gerry Chang [:gchang]]

Comment on attachment 8808509 [details] [diff] [review]
bug_130147_remove_release_assert.patch

Fix a crash. Aurora51+.

Comment 72

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c8d0b43d402

Comment 73

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: annotate LoadExtendedValidationInfo failures

I have no good explanation for this, so here's more debugging info.
The bad news is this isn't why this code is failing in 50/51, because this code is only here because of a change made in 52 (see bug 1227638). But maybe once we figure this failure out we can find the underlying failure.

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d6f69e0837
add annotated crashes to find out why PSM initialization is failing r=dragana

Comment 75

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Thanks! We'll see how this goes.
(This was the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=480e52ffa4ecd8797b6f6e70df7848c53475092a )

Comment 76

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

https://hg.mozilla.org/mozilla-central/rev/68d6f69e0837

Comment 77

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to Dragana Damjanovic [:dragana] from comment #70)
> It is crashing at:
> 1861 rv = LoadExtendedValidationInfo();
> 1862 #ifdef ANDROID
> 1863 MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));
> 1864 #endif
> https://hg.mozilla.org/mozilla-central/annotate/336759fad462/security/
> manager/ssl/nsNSSComponent.cpp#l1863
> 
> https://crash-stats.mozilla.com/signature/?version=52.
> 0a1&signature=nsNSSComponent%3A%3AInitializeNSS&date=%3E%3D2016-11-
> 04T09%3A40%3A05.000Z&date=%3C2016-11-11T09%3A40%3A05.
> 000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_colum
> ns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports

Hmmm - crash stats may have been giving us misleading information here. The report does link to that line of source code, but the "crash reason" is "MOZ_RELEASE_ASSERT(init_rv == SECSuccess)", which would point to this line:

https://hg.mozilla.org/mozilla-central/annotate/336759fad462/security/manager/ssl/nsNSSComponent.cpp#l1823

This makes a bit more sense, because there's really nothing in LoadExtendedValidationInfo that would be failing in some configurations but not others (other than out-of-memory errors, but I imagine those wouldn't be consistently failing in the same place).  That said, I'm also not aware of any reason NSS_NoDB_Init would fail, because that should be doing memory-only operations that don't depend on any previous or external state.

Comment 78

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

Too late to fix in 50.1.0 release

Comment 79

[Dragana Damjanovic [:dragana]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #77)
> (In reply to Dragana Damjanovic [:dragana] from comment #70)
> > It is crashing at:
> > 1861 rv = LoadExtendedValidationInfo();
> > 1862 #ifdef ANDROID
> > 1863 MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));
> > 1864 #endif
> > https://hg.mozilla.org/mozilla-central/annotate/336759fad462/security/
> > manager/ssl/nsNSSComponent.cpp#l1863
> > 
> > https://crash-stats.mozilla.com/signature/?version=52.
> > 0a1&signature=nsNSSComponent%3A%3AInitializeNSS&date=%3E%3D2016-11-
> > 04T09%3A40%3A05.000Z&date=%3C2016-11-11T09%3A40%3A05.
> > 000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_colum
> > ns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports
> 
> Hmmm - crash stats may have been giving us misleading information here. The
> report does link to that line of source code, but the "crash reason" is
> "MOZ_RELEASE_ASSERT(init_rv == SECSuccess)", which would point to this line:
> 
> https://hg.mozilla.org/mozilla-central/annotate/336759fad462/security/
> manager/ssl/nsNSSComponent.cpp#l1823
> 

You are right.
We should probably try to narrow this down when we already went so far.
David, can you make a patch?

Comment 80

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: 1301407-1_3-backout.diff

Since NSS_NoDB_Init is NSS code and it's not clear where to start looking for the problem, I thought it might make sense to get some annotated crash reports that tell us what the PR error code from the failure was. With any luck, that will be specific enough to narrow down the search.

At the same time, I'm going to back out some of the now-unnecessary patches with the android-specific release assertions.

Comment 81

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: 1301407-2_3-backout.diff

Comment 82

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: 1301407-3_3-annotate-NSS_NoDB_Init.diff

Comment 83

[Dragana Damjanovic [:dragana]]

Thanks!

Comment 84

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0bd0f0e2b53
Backed out changeset 68d6f69e0837 for not being necessary any longer r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bfeb3cf38a
Backed out changeset 425f66fa9b10 for not being necessary any longer r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f2799c3990
capture NSS_NoDB_Init failure error codes in crash reports r=dragana

Comment 85

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Thanks!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdd258a8782518e09fd59c271e64f3bff91ae95b

Comment 86

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

https://hg.mozilla.org/mozilla-central/rev/15f2799c3990

Comment 87

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

(The backouts merged to m-c, too.)
https://hg.mozilla.org/mozilla-central/rev/f0bd0f0e2b53
https://hg.mozilla.org/mozilla-central/rev/b9bfeb3cf38a

Comment 88

[David Bolter [:davidb] (NeedInfo me for attention)]

What's next for this bug?

Comment 89

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

The 3 reports I can find that are from the most recent changes have the annotation "NSS_NoDB_Init failed with PRErrorCode -5925". -5925 is PR_CALL_ONCE_ERROR. Presumably a PR_CallOnce call failed in an earlier call to NSS_Initialize. Unfortunately the API doesn't save the original error, so we'll have to grab them manually and include them in the annotation if we want to know what it is. That might lead us to what's actually failing.

Comment 90

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: 1301407-save-prerror.diff

Comment 91

[Dragana Damjanovic [:dragana]]

Comment on attachment 8830515 [details] [diff] [review]
1301407-save-prerror.diff

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

Thanks.

Comment 92

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54cecb685bca
save PRErrorCode from all NSS initialization calls and include in annotated crash report r=dragana

Comment 93

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

https://hg.mozilla.org/mozilla-central/rev/54cecb685bca

Comment 94

[Ryan VanderMeulen [:RyanVM]]

Looks like all the recent reports with this signature have been on desktop. Should we maybe uplift the last patch further up the chain to see if we get more useful reports?

Comment 95

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

For some reason I had the impression this was android-only, but it looks like that's not the case. Since I'm not seeing any reports for the most recent patch, perhaps the thing to do would be to take out the #ifdef ANDROID bits and see if we get more information.

Comment 96

[Randell Jesup [:jesup] (needinfo me)]

From one of the Windows crashes, it's crashing in an assert:
 MOZ_RELEASE_ASSERT(NS_IsMainThread());

Comment 97

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to Randell Jesup [:jesup] from comment #96)
> From one of the Windows crashes, it's crashing in an assert:
>  MOZ_RELEASE_ASSERT(NS_IsMainThread());

Right - for platforms without the diagnostic crash reporting code, the first (every?) attempt at initialization (on the main thread) seems to fail. It then appears that something off the main thread uses something that causes nsNSSComponent to be initialized, but since it's not the main thread, that assertion fails.

I'm hoping bug 1339267 will fix this, but since that's not quite ready, we might as well enable the diagnostic crash reporting on all platforms and see if it sheds any more light on this.

Comment 98

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: 1301407-enable-on-all-platforms.diff

Comment 99

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd8df20768f
enable nsNSSComponent initialization diagnostic crash report on all platforms r=dragana

Comment 100

[Carsten Book [:Tomcat]]

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

Comment 101

[Dragana Damjanovic [:dragana]]

Is this bug fixed by bug 1339267, or should we do something in addition?

Comment 102

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

I think so - I'm not seeing any crashes with this signature after when that landed. I'm going to keep looking for a bit, though.

Comment 103

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 104

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

At this point, it's looking like this was fixed by bug 1339267. (The error codes from the few reports received after this landed correspond to SEC_ERROR_LEGACY_DATABASE and SEC_ERROR_PKCS11_DEVICE_ERROR. If a user encounters these at this point, really the only thing they can do is start in safe mode or try with a new profile.)

Comment 105

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.