1767589 - Crash in [@ PinCurrentAppToTaskbarImpl]

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect, P1)

Description

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

121 crashes from 44 installations of 101.0a1 and 101.0b1 and b2, most on Windows 8.1, some on Windows 7. First affected build is 101.0a1 20220427094429 which points to bug 1763573 as regressing change.

Crash report: https://crash-stats.mozilla.org/report/index/dd2c0779-d048-4eb4-8263-563a40220428

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll PinCurrentAppToTaskbarImpl browser/components/shell/nsWindowsShellService.cpp:1346
1 xul.dll XPTC__InvokebyIndex 
2 xul.dll _tailMerge_d3dcompiler_47.dll 
3 xul.dll static XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1125
4 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:963
5 xul.dll Interpret js/src/vm/Interpreter.cpp:3314
6 xul.dll js::CallGetter js/src/vm/Interpreter.cpp:731
7 xul.dll js::NativeGetProperty js/src/vm/NativeObject.cpp:2172
8 xul.dll js::jit::DoGetElemFallback js/src/jit/BaselineIC.cpp:648
9 None @0x00000094fe9176ae 

Comment 1

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

:aryx, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: Bug 1767589: Fix BSTR handling in nsWindowsShellService r?rkraesig

The crash stacks we have for this bug actually point the finger at the calling function of what I'm modifying here (PinCurrentAppToTaskbarImpl) -- but I can't see anything in there causing an access violation.

If I understand this code correctly, there is some potential for one here though. Most notably:

• The first BStrPtr never allocates anything (but will still try to free it?)
• Subsequent overwrites of name may cause the originals never to be freed(?)

There may be additional or different changes needed to fix this crash, but this patch seems like it's fixing another issue at the very least.

Comment 3

[[e-mail] (:bhearsum)]

Attachment: Bug 1767589: check for S_FALSE where appropriate in PinCurrentAppToTaskbar r?rkraesig

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5492a8c99f4a
check for S_FALSE where appropriate in PinCurrentAppToTaskbar r=rkraesig

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5492a8c99f4a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Hi Ben, please nominate this for Beta approval so we can get it into today's b5 build.

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 9275311 [details]
Bug 1767589: check for S_FALSE where appropriate in PinCurrentAppToTaskbar r?rkraesig

Beta/Release Uplift Approval Request

• User impact if declined: Crashes for some users
• Is this code covered by automated tests?: No
• 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 patch just adds a safeguard around some existing code.
• String changes made/needed:
• Is Android affected?: No

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9275311 [details]
Bug 1767589: check for S_FALSE where appropriate in PinCurrentAppToTaskbar r?rkraesig

Approved for 101.0b5.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/adf37cc4e40e

Comment 10

[Alex Finder]

Hello! We encountered the following regression for talos on mozilla-beta. Would it be possible for this push to have triggered that regression ?

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Alex Finder from comment #10)

Hello! We encountered the following regression for talos on mozilla-beta. Would it be possible for this push to have triggered that regression ?

This is very, very unlikely to be the cause of this regression. The code in question is not used as part of page loads.