1779040 - Crash in [@ shutdownhang | mozilla::TaskQueue::AwaitShutdownAndIdle]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Not a new crash signature. 900 crashes from Windows installations for Firefox 101.

Crash report: https://crash-stats.mozilla.org/report/index/6d9d0e3b-0e4c-4580-847c-5b2be0220711

MOZ_CRASH Reason: Shutdown hanging at step AppShutdown. Something is blocking the main-thread.

Top 10 frames of crashing thread:

0 ntdll.dll NtWaitForAlertByThreadId 
1 ntdll.dll RtlSleepConditionVariableSRW 
2 KERNELBASE.dll SleepConditionVariableSRW 
3 mozglue.dll mozilla::detail::ConditionVariableImpl::wait mozglue/misc/ConditionVariable_windows.cpp:50
4 xul.dll mozilla::TaskQueue::AwaitShutdownAndIdle xpcom/threads/TaskQueue.cpp:185
5 xul.dll mozilla::DataStorage::Observe security/manager/ssl/DataStorage.cpp:794
6 xul.dll nsObserverList::NotifyObservers xpcom/ds/nsObserverList.cpp:70
7 xul.dll nsObserverService::NotifyObservers xpcom/ds/nsObserverService.cpp:291
8 xul.dll static mozilla::AppShutdown::AdvanceShutdownPhaseInternal xpcom/base/AppShutdown.cpp:372
9 xul.dll nsXREDirProvider::DoShutdown toolkit/xre/nsXREDirProvider.cpp:985

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Hey Dana, could you please take a quick look at this one?

Comment 2

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

I think the thread writing out DataStorage data is essentially taking too long. In other words, we'll probably have to do something like bug 1610136 to address these crashes.

Comment 3

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 20 desktop browser crashes on release

For more information, please visit auto_nag documentation.

Comment 4

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

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit auto_nag documentation.

Comment 5

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

Attachment: Bug 1779040 - don't update DataStorage if HSTS data hasn't significantly changed r?jschanck

When gecko encounters multiple responses from the same host with substantially
the same HSTS information, the implementation shouldn't update DataStorage,
because that can cause unnecessary writes. "Substantially the same" means the
information is identical except for the expiration time, which can be up to a
day different.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7693eca175e9
don't update DataStorage if HSTS data hasn't significantly changed r=jschanck

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7693eca175e9

Comment 8

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

The patch landed in nightly and beta is affected.
:keeler, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

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

Comment on attachment 9299517 [details]
Bug 1779040 - don't update DataStorage if HSTS data hasn't significantly changed r?jschanck

Beta/Release Uplift Approval Request

• User impact if declined: Potential shutdown hangs.
• 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): Relatively small change, has a test.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9299517 [details]
Bug 1779040 - don't update DataStorage if HSTS data hasn't significantly changed r?jschanck

Approved for 107.0b7.

Comment 11

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/35c2f8c0542b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks like the only remaining instances of this crash are on ESR102, but we'd need a rebased patch if we wanted to uplift there. What are your thoughts, Dana?

Comment 13

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

I think it would be reasonable to uplift to ESR102. I'll prepare a patch.

Comment 14

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

Attachment: patch-for-esr102.diff

ESR Approval Request Comment
[Feature/Bug causing the regression]: long-standing issue
[User impact if declined]: potential shutdown hangs
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not particularly - it's well-contained and tested
[Why is the change risky/not risky?]: ^
[String changes made/needed]: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9306138 [details] [diff] [review]
patch-for-esr102.diff

Approved for 102.6esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/448e55025b51