1452204 - Does synchronous stackwalking in profiler_get_backtrace not work on Windows?

Status: RESOLVED FIXED

(Core :: mozglue, defect, P2)

Description

[Markus Stange [:mstange]]

In this profile: https://perfht.ml/2GZCPDA
there are a few styles and reflow markers which have cause callstacks. However, the C++ stacks in there look rather odd. They are all only two functions deep:

profiler_get_backtrace()   xul.pdb
RtlAllocateMemoryBlockLookaside   ntdll.pdb

Is synchronous unwinding of the native stack just broken on Windows at the moment?

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I think Xidron said he would try to take a look at this when he has some free cycles?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

OK, I guess I know what the problem is.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1452204 part 1 - Correctly set walkCallingThread.

GetCurrentThread() returns a pseudo handle, so comparing it against
the passed in argument doesn't make sense in most cases. This patch
changes it to using the thread id for comparison, which is guaranteed
to be unique in the whole lifetime of a thread.

Review commit: https://reviewboard.mozilla.org/r/248864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248864/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1452204 part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread.

GetThreadContext() returns a context pointing to its own frame when it
gets called with the current thread handle. That frame can go away after
it returns. This patch instead uses RtlCaptureContext(), which captures
the context of its caller, when walking the current thread.

Review commit: https://reviewboard.mozilla.org/r/248866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248866/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983010 [details]
Bug 1452204 part 1 - Correctly set walkCallingThread.

GetCurrentThread() returns a pseudo handle, so comparing it against
the passed in argument doesn't make sense in most cases. This patch
changes it to using the thread id for comparison, which is guaranteed
to be unique in the whole lifetime of a thread.

Review commit: https://reviewboard.mozilla.org/r/248864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248864/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983011 [details]
Bug 1452204 part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread.

GetThreadContext() returns a context pointing to its own frame when it
gets called with the current thread handle. That frame can go away after
it returns. This patch instead uses RtlCaptureContext(), which captures
the context of its caller, when walking the current thread.

Review commit: https://reviewboard.mozilla.org/r/248866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248866/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8983010 [details]
Bug 1452204 part 1 - Correctly set walkCallingThread.

https://reviewboard.mozilla.org/r/248864/#review255274

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8983011 [details]
Bug 1452204 part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread.

https://reviewboard.mozilla.org/r/248866/#review255282

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72fc40daf6cd
part 1 - Correctly set walkCallingThread. r=glandium
https://hg.mozilla.org/integration/autoland/rev/4431cecd4c2d
part 2 - Use RtlCaptureContext to capture context for current thread. r=glandium

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a47957da1983
Backed out 2 changesets for perma failing in memory/replace/dmd/test/test_dmd.js

Comment 11

[Eliza Balazs [:ebalazs_]]

Backed out 2 changesets (bug 1452204) for perma failing in memory/replace/dmd/test/test_dmd.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4431cecd4c2d43fedc0acfa5016ef89a583d87b6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=181841156

Backout: Backed out 2 changesets (bug 1452204) for perma failing in memory/replace/dmd/test/test_dmd.js

Failure log: Backed out 2 changesets (bug 1452204) for perma failing in memory/replace/dmd/test/test_dmd.js

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

OK, eventually I figured out what's going on.

It seems we can remove quite a bit of code for walking the current thread.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983010 [details]
Bug 1452204 part 1 - Correctly set walkCallingThread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248864/diff/1-2/

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983011 [details]
Bug 1452204 part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248866/diff/1-2/

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983011 [details]
Bug 1452204 part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread.

This change is non-trivial and definitely needs another review.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9e24b02203f788a10bf533f30e2d510726aec03

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8983011 [details]
Bug 1452204 part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread.

https://reviewboard.mozilla.org/r/248866/#review256098

::: commit-message-4087b:8
(Diff revision 2)
> +GetThreadContext() returns a context pointing to its own frame when it
> +gets called with the current thread handle. That frame can go away after
> +it returns. This patch instead uses RtlCaptureContext(), which captures
> +the context of its caller, when walking the current thread.
> +
> +In the past, we also use a walker thread when nullptr is passed in for

s/use/used/

::: commit-message-4087b:10
(Diff revision 2)
> +it returns. This patch instead uses RtlCaptureContext(), which captures
> +the context of its caller, when walking the current thread.
> +
> +In the past, we also use a walker thread when nullptr is passed in for
> +aThread, but the check doesn't cover all the cases, and having another
> +thread is apparently more complicated then this approach.

s/then/than/

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983011 [details]
Bug 1452204 part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248866/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6df1a71c5fe2
part 1 - Correctly set walkCallingThread. r=glandium
https://hg.mozilla.org/integration/autoland/rev/12b8065ee9fc
part 2 - Use RtlCaptureContext to capture context for current thread and remove walker thread. r=glandium

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/6df1a71c5fe2
https://hg.mozilla.org/mozilla-central/rev/12b8065ee9fc