Closed Bug 1620549 Opened 2 years ago Closed 2 years ago

Profiler overflows stack when default thread stack size is < 160 KiB

Categories

(Core :: Gecko Profiler, defect, P1)

73 Branch
All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: mforney, Assigned: mforney)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

I built firefox for Linux with musl libc (testing a fix for bug 1615713), and started the profiler.

Actual results:

The profiler crashed due to a stack overflow, since it tries to allocate 160 KiB for backtrace information, but musl's default thread stack size is only 128 KiB (http://git.musl-libc.org/cgit/musl/tree/src/internal/pthread_impl.h#n182).

Expected results:

The profiler should have set the minimum stack size for the sampling thread to be at least as large as it needs to store N_STACK_BYTES to prevent the overflow.

This can be done with pthread_attr_setstacksize.

N_STACK_BYTES are needed to store backtrace information, so use
that plus some extra as the minimum stack size for the sampler
thread to ensure that it doesn't overflow.

Assignee: nobody → mforney
Priority: -- → P1
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9930f4d0e821
Set stack size of profiler sampler thread to N_STACK_BYTES + 20 KiB r=gerald,mstange
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/4074abe0bef9
Set stack size of profiler sampler thread to N_STACK_BYTES + 20 KiB r=gerald,mstange
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4599809c6e0
Backed out changeset 4074abe0bef9 for asan failures on platform.cpp . CLOSED TREE

Narcis, could you provide logs of the ASAN failures? I don't know how I can reproduce them myself.

Flags: needinfo?(mforney) → needinfo?(nbeleuzu)
Attachment #9131429 - Attachment description: Bug 1620549 - Set stack size of profiler sampler thread to N_STACK_BYTES + 20 KiB → Bug 1620549 - Set stack size of profiler sampler thread to N_STACK_BYTES + 80 KiB

I was able to reproduce the issue. It looks like the sampler thread needs at least N_STACK_BYTES + 38 KiB, so I bumped the stack size to N_STACK_BYTES + 80 KiB to keep a safe margin above that.

Flags: needinfo?(nbeleuzu)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Unspecified → Linux
Hardware: Unspecified → All
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d40b5da02500
Set stack size of profiler sampler thread to N_STACK_BYTES + 80 KiB r=gerald,mstange

Okay, I managed to reproduce the error when building with the TSAN configuration given in
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Thread_Sanitizer#Adjusting_the_build_configuration

It appears that when built with TSAN, at least 292 KiB extra stack space (in addition to the 160 KiB for N_STACK_BYTES) are needed, though I don't understand why.

Markus, could you advise on an appropriate stack size for the sampler thread? Is this much stack usage expected? My next guess would be to try N_STACK_BYTES + 800 KiB (around 8 times as big as musl's default thread stack size), but my previous guesses have been way off.

Are there any other build configurations I should test?

Flags: needinfo?(mforney) → needinfo?(mstange)

I don't know. I don't have much guidance on how to find good stack sizes. I think our general approach has been "start small and then double it every time we run into a problem, until we no longer run into problems"...
I don't know how TSAN works or through what mechanism it places extra stuff on the stack.

Flags: needinfo?(mstange)
Attachment #9131429 - Attachment description: Bug 1620549 - Set stack size of profiler sampler thread to N_STACK_BYTES + 80 KiB → Bug 1620549 - Set stack size of profiler sampler thread to 800 KiB

Okay, let's try 800 KiB then. I updated the phabricator patch.

Markus, can you run this through the try server? I don't want to risk this getting rolled back again.

Flags: needinfo?(mstange)

Ah, sorry I missed your message on Phabricator. I've started a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95f4cd62c4f698b401fffb396547231cf1f04adf

Flags: needinfo?(mstange)
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7daa6afda13
Set stack size of profiler sampler thread to 800 KiB r=gerald,mstange
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
You need to log in before you can comment on or make changes to this bug.