Closed Bug 1355807 Opened 2 years ago Closed 2 years ago

MOZ_RELEASE_ASSERT in PseudoStack::stopJSSampling fails

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jseward, Assigned: njn)

References

Details

Crash Data

Attachments

(2 files)

This fails:

    MOZ_RELEASE_ASSERT(mJSSampling == ACTIVE ||
                       mJSSampling == ACTIVE_REQUESTED);

STR:
* release build of m-c on x86_64-Linux (but this looks target independent)
* start up.  Set profiler params to: 18MB buffer, 1ms sampling, "," (all threads).
* quit
* restart, check that abovementioned params have "stuck"
* In the thread filter box, change the "," to "GeckoMain,Compositor"
* click twice on "restart profiler".  It segfaults reliably after the
  second click.
* If that doesn't work, just click as fast as you can on "restart profiler"
  having changed the text in the box.

Getting a stack trace is a bit tricky because it crashes whilst the profiler
menu holds the mouse focus (at least for me), leaving the whole display unusable.
In the end I ran it in an Xvnc server.
Attached file Stacktrace
Flags: needinfo?(n.nethercote)
(In reply to Julian Seward [:jseward] from comment #0)
> * release build of m-c on x86_64-Linux (but this looks target independent)

Yes.  The same STR also causes a segfault on MacOS.
Duplicate of this bug: 1355963
Crash Signature: [@ locked_profiler_stop]
I've diagnosed the problem and have fixed it locally. Now I want to write a test case for it.
Flags: needinfo?(n.nethercote)
PseudoStack requires that startJSSampling() and stopJSSampling() calls be
interleaved. But currently the conditions guarding those calls don't match:
startJSSampling() is guarded by ShouldProfileThread(), and stopJSSampling() is
guarded by HasProfile().

It's possible for HasProfile() to be true when ShouldProfileThread() is not
true -- e.g. profile many threads, then restart and profile fewer threads, and
we end up with live threads that have a profile but aren't being profiled right
now -- which leads to assertion failures in stopJSSampling().

This patch makes the stopJSSampling() condition use ShouldProfileThread(), just
like the startJSSampling() condition, which fixes the assertion failure.
Attachment #8857708 - Flags: review?(jseward)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Duplicate of this bug: 1356052
Attachment #8857708 - Flags: review?(jseward) → review+
Crash Signature: [@ locked_profiler_stop] → [@ locked_profiler_stop] [@ PseudoStack::stopJSSampling ]
https://hg.mozilla.org/mozilla-central/rev/e295801c2da9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.