1788859 - Stack-walking may cause deadlocks during calls to patched LdrLoadDll's

Status: RESOLVED DUPLICATE of bug 1836225

(Core :: Gecko Profiler, defect, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Spawned from bug 1687510 comment 7:

I think I just got the deadlock (win10/x64 local build). I attached a debugger, the main thread was stuck in the interceptor while patched_LdrLoadDll was calling stub_LdrLoadDll().
We could add AutoSuppressStackWalking in these patched_... functions. It may not fix the suppression-check race, but it would help remove places where it could happen.

I'm leaving bug 1687510 open, as it shows some potential race (see bug 1687510 comment 5) that should probably be handled there.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1788859 - Suppress stack-walking while calling LdrLoadDll in WindowsDllBlocklist.cpp - r?florian

Stack-walking during some Windows API calls result in deadlocks, see details in StackWalk.cpp.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2676d5d34684
Suppress stack-walking while calling LdrLoadDll in WindowsDllBlocklist.cpp - r=florian

Comment 3

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2676d5d34684

Comment 4

[Yannis Juglaret [:yannis] (Away until April 13)]

Attachment: Backed out changeset 2676d5d34684 (Bug 1788859) because it is no longer required. r=handyman,florian

This changeset from bug 1788859 was an attempt at mitigating bug
1687510. With the proper fix coming in bug 1836225, this changeset is no
longer required. The stack walk suppressions that this changeset
adds are already on a stack walk suppressed path, as guarded by the
scope of the surrounding glue::ModuleLoadFrame. Indeed:

• the constructor for glue::ModuleLoadFrame calls
  sLoaderAPI->ConstructAndNotifyBeginDllLoad;

• LoaderPrivateAPIImp::ConstructAndNotifyBeginDllLoad and
  FallbackLoaderAPI::ConstructAndNotifyBeginDllLoad both end up calling
  gLoaderObserver->OnBeginDllLoad or mLoaderObserver->OnBeginDllLoad;

• in both cases, the LoaderObserver is gMozglueLoaderObserver, as is
  guaranteed by the call to glue::ModuleLoadFrame::StaticInit in
  DllBlocklist_Initialize (which is the function that sets up this
  fallback patched_LdrLoadDll hook in case the launcher process failed
  to set up the freestanding patched_LdrLoadDll hook).

Depends on D181437

Comment 5

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6a41bd11965
Backed out changeset 2676d5d34684 because it is no longer required. r=handyman,florian

Comment 7

[Cosmin Sabou [:CosminS]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/f6a41bd11965

Normally we reopen backed out bugs but given this was fixed in 106 branch maybe you got other plans and will leave this for you to decide.

Comment 8

[Yannis Juglaret [:yannis] (Away until April 13)]

Maybe I should not have used a backout to remove that code, but the situation is under control, the patches in bug 1836225 fix the issue that was observed here. Resolving as duplicate which is maybe the best way to express that.