1650654 - Crash in [@ PK11_TraverseCertsInSlot]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-802f5a47-ae5b-445f-ba81-295a30200705.

Top 10 frames of crashing thread:

0 nss3.dll PK11_TraverseCertsInSlot security/nss/lib/pk11wrap/pk11cert.c:2353
1 nss3.dll PK11_ListCertsInSlot security/nss/lib/pk11wrap/pk11cert.c:2893
2 xul.dll IntermediatePreloadingHealerCallback security/manager/ssl/nsNSSComponent.cpp:2186
3 xul.dll nsTimerImpl::Fire xpcom/threads/nsTimerImpl.cpp:562
4 xul.dll nsTimerEvent::Run xpcom/threads/TimerThread.cpp:251
5 xul.dll mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:158
6 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:299
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
8 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:504
9 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:332

this crash signature is starting to show up in the 79.0b cycle and looks related to the changes from bug 1630434.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Crash volume looks a bit concerning.

Comment 2

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

Ryan, it looks like our best next step here would be to land a diagnostic assert directly into beta's copy of NSS, and pull it out after we get some more data. Such an edit-to-the-RTM-tagged-NSS move wouldn't work for Linux, but it appears to be pretty much only Windows, where that kind of trickery works.

Comment 3

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

Attachment: Bug 1650654 - diagnostic assertions to narrow down where PK11_TraverseCertsInSlot is crashing r?jcj

Comment 4

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

Comment on attachment 9162009 [details]
Bug 1650654 - diagnostic assertions to narrow down where PK11_TraverseCertsInSlot is crashing r?jcj

Beta/Release Uplift Approval Request

• User impact if declined: we're using this to (hopefully) narrow down where/why PK11_TraverseCertsInSlot is crashing when we try to run the intermediate preloading healer
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): this should be low-risk - the behavior we're changing is how/where we crash if NSS would dereference a null pointer (so, this will crash if and only if it would already crash)
  also, we're going to back this out before release
• String changes made/needed:

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9162009 [details]
Bug 1650654 - diagnostic assertions to narrow down where PK11_TraverseCertsInSlot is crashing r?jcj

Diagnostic asserts to hopefully track down the cause of this crash. Approved for 79.0b5.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9162009 [details]
Bug 1650654 - diagnostic assertions to narrow down where PK11_TraverseCertsInSlot is crashing r?jcj

Uplifted to Beta. Clearing the approval flag to get it off the needs-uplift radar.
https://hg.mozilla.org/releases/mozilla-beta/rev/68fb048ce9ee

Comment 7

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on devtoken.c

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/d3460ec103e819b35b1665be7fb055a82537cb91
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308893758&repo=mozilla-beta&lineNumber=34885

Comment 8

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

who knew that kernel32 wasn't around?

I'm debugging this in the phabricator review... pretty much debug-by-trypush, nothing elegent.

Comment 9

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

Comment on attachment 9162009 [details]
Bug 1650654 - diagnostic assertions to narrow down where PK11_TraverseCertsInSlot is crashing r?jcj

Beta/Release Uplift Approval Request

• User impact if declined: (please see comment 4)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): looks like this should build on all of our windows platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=efd850da4962d4fa79a93ce71caa5d8aaba183ba
• String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9162009 [details]
Bug 1650654 - diagnostic assertions to narrow down where PK11_TraverseCertsInSlot is crashing r?jcj

Approved for 79.0b6.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9162009 [details]
Bug 1650654 - diagnostic assertions to narrow down where PK11_TraverseCertsInSlot is crashing r?jcj

Landed on Beta for 79.0b6.
https://hg.mozilla.org/releases/mozilla-beta/rev/5f02771d855b

Removing the approval flag to get this off the needs-uplift radar.

Comment 12

[[:philipp]]

loads of reports crashing on the added assertion are already coming in after 79.0b6 got released - so far all of them are crashing here: https://hg.mozilla.org/releases/mozilla-beta/annotate/49c7a261c59c4c5ace83bc2d727d7eef0dd29e9c/security/nss/lib/dev/devslot.c#l150

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for 79.0b7 per discussion with Dana and JC.
https://hg.mozilla.org/releases/mozilla-beta/rev/b479804e64c1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Beta79 is no longer affected by this due to bug 1651155 disabling the regressing feature.

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

This will also be disabled as of 80.0b7 in a couple of weeks.

Comment 16

[Jens Stutte [:jstutte]]

Hi Dana, can we assume that this remains disabled also for 81 late beta?

Comment 17

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

Yes - this will probably remain disabled for the foreseeable future while we re-work some aspects of how gecko uses NSS.

Comment 18

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

The leave-open keyword is there and there is no activity for 6 months.
:keeler, maybe it's time to close this bug?

Comment 19

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

Yeah, I'm not sure it makes sense to keep this bug open.

Comment 20

[Mathew Hodson]

security.intermediate_preloading_healer.enabled is still set to true in Nightly right? So is this crash still happening there or was it fixed in another bug?