1778444 - Exit crash in static ComPtr<IUISettings5> sUiSettings;

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Kai Engert (:KaiE:)]

Only on Windows 11, we see this bug with Thunderbird 102, but I cannot reproduce it using Firefox 102.

Original report is in bug 1773392, but I'm filing this bug for Widget, because this is where the origin of the crash is.

The problem happens when Thunderbird is started using -P, the profile manager comes up, and when profile manager exits, we run into a cleanup crash here:
https://searchfox.org/mozilla-esr102/source/widget/windows/WindowsUIUtils.cpp#339

The VS debugger allows me to see two more stack levels, because that's code in Windows headers. It crashes below client.h, inside ComPtr::InternalRelease, line temp->Release(), with access violation, reading location 0x000...00070.

See also the crash reports in the other bug.

Comment 1

[Kai Engert (:KaiE:)]

I couldn't find any obvious reference counting etc. bug in the related code.

At the time we reach the destructor of that static instance (inside the static function, called from atexit), the COMPtr still points to the original value, so no apparent memory corruption.

Maybe we're too late in execution, and too much of the process has already been cleaned up?

Our code comment says:
// We need to keep this alive for ~ever so that change callbacks work as
// expected, sigh.

If you want to keep it forever anyway, can we leak it?
I'm attaching a patch that leaks, and no longer crashes.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Bug 1778444 - Leak sUiSettings to avoid shutdown crash (esr102 patch). r=emilio

Comment 3

[Kai Engert (:KaiE:)]

Mike, do you know if this is safe or expected to crash at exit?

class _class {
  static void _method() {
    static ComPtr<ISomething> s;
  }
}

Comment 4

[Emilio Cobos Álvarez (:emilio)]

My guess is that we're keeping the non-null ComPtr past MSCOM shutdown and for some reason that causes trouble (but why on TB and not on Firefox?)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Hmm, do we even shut down MSCOM?

Comment 6

[James Teh [:Jamie]]

I think we shut down mscom here.

Is there any reason we can't use ClearOnShutdown to deal with this pointer? Surely we don't need these change callbacks post XPCOM shutdown?

Comment 7

[James Teh [:Jamie]]

CoUninitialize (called when we shut down mscom) potentially releases COM references tracked by the COM runtime, so it is reasonable (if unfortunate) that releasing a COM pointer after that would crash.

Comment 8

[James Teh [:Jamie]]

Actually, if we don't run the skeleton UI (I'm guessing the profile manager might not? I'm not sure), we might clean up mscom when exiting the scope of XREMain::XRE_main, since we create an instance in that function. Either way, I think ClearOnShutdown should fix this.

Comment 9

[Wayne Mery (:wsmwk)]

FWIW, please note that for bug 1773392 30% of crashes are <=3 seconds uptime and 80% <=30 seconds.

The one user story we have is from bug 1777777 ...

"Started Thunderbird 102 with /profilemanager switch. The Profile dialog appears, then after launching with a selected profile, a crash dialog appears. Thunderbird 102 starts anyway and it is possible to use."

As Kai notes, it is strange this doesn't reproduce for Firefox.

Comment 10

[Kai Engert (:KaiE:)]

(In reply to James Teh [:Jamie] from comment #8)

Either way, I think ClearOnShutdown should fix this.

Doesn't work, I get compile error "no matching function"....

I have the impression this function is only supposed to work with Mozilla-code smartpointers, but not with the MS ComPtr.

Comment 11

[Kai Engert (:KaiE:)]

The difference between FF and TB could potentially be caused by some TB specific code, that enforces a master password prompt for TB on startup, and that could cause different init orders.

Comment 12

[Kai Engert (:KaiE:)]

The problematic code is code is behind a pref: widget.windows.overlay-scrollbars.enabled

Maybe Thunderbird could disable that pref temporarily, until we're ready to uplift the fix to the mozilla-esr102 branch?
(And re-enable the pref in TB later after uplifting the code fix.)

I'll suggest that in the TB crash bug, let me know if you have any concerns regarding that.

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:KaiE:) from comment #12)

Maybe Thunderbird could disable that pref temporarily, until we're ready to uplift the fix to the mozilla-esr102 branch?

Actually, that might not help. If we're starting the profile manager, it will always use the default pref value.

Comment 14

[Kai Engert (:KaiE:)]

Confirming we also crash with nightly Thunderbird based on comm-central.
(I built that locally, too, to allow me to test the m-c patch.)

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Bug 1778444 - Leak sUiSettingsAsInspectable to avoid shutdown crash. r=emilio

Comment 16

[Kai Engert (:KaiE:)]

Second attachment is the fix for mozilla-central, which I've confirmed fixes the crash with comm-central (Thunderbird daily).

Comment 17

[Kai Engert (:KaiE:)]

(In reply to Wayne Mery (:wsmwk) from comment #9)

FWIW, please note that for bug 1773392 30% of crashes are <=3 seconds uptime and 80% <=30 seconds.

This makes sense wrt profile manager, most users would be quick in selecting what they want to do there. If profile manager is confirmed, a process for the profile manager exits, and a new process with the selected profile is started.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1778444 - Clear UISettings on shutdown. r=kaie

This should be the right fix.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1778444 - Clear UISettings on shutdown (esr). r=kaie

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Kai, if you could test comment 18 on TB to confirm it fixes stuff it'd be great. I think that's a bit better fix than just perma-leaking the object past MSCOM shutdown.

Comment 21

[Kai Engert (:KaiE:)]

Thanks Emilio!

Both patches tested and confirmed to fix the crash.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9284591 [details]
Bug 1778444 - Clear UISettings on shutdown (esr). r=kaie

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Thunderbird crash fix.
• User impact if declined: See blocked bug.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple tweak to avoid destroying COM objects past shutdown.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed78195edff9
Clear UISettings on shutdown. r=kaie

Comment 24

[Iulian Moraru]

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

Comment 25

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

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

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

For more information, please visit auto_nag documentation.

Comment 26

[Kai Engert (:KaiE:)]

Comment on attachment 9284585 [details]
Bug 1778444 - Clear UISettings on shutdown. r=kaie

Beta/Release Uplift Approval Request

• User impact if declined: Thunderbird crashes.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: Yes
• 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 simple refactoring.
• String changes made/needed:
• Is Android affected?: No

Comment 27

[Donal Meehan [:dmeehan]]

Comment on attachment 9284585 [details]
Bug 1778444 - Clear UISettings on shutdown. r=kaie

Approved for 103.0b7, thanks.

Comment 28

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20dd1e80b209

Comment 29

[Donal Meehan [:dmeehan]]

Comment on attachment 9284591 [details]
Bug 1778444 - Clear UISettings on shutdown (esr). r=kaie

Approved for 102.1esr.

Comment 30

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/0b455b86c560

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Pushed to FIREFOX_ESR_102_0_X_RELBRANCH per request from the TB team for TB 102.0.3.
https://hg.mozilla.org/releases/mozilla-esr102/rev/0faf039283ce