Steps to reproduce: 1. Use the add-on from https://perf-html.io/ to profile your browser with the default settings. 2. Notice that the Compositor thread shows up in profiles that you grab with these settings. 3. Open the panel, remove the part ",Compositor" from the threads textbox (so that it's just "GeckoMain"), and click "Apply (Restart Profiler)". 4. Grab another profile. Expected results: No more Compositor thread in the profile. Actual results: This new profile still contains the Compositor thread.

It looks like ThreadInfo only has a way to set mHasProfile to true (using ThreadInfo::SetHasProfile()), it's never set to false again. We should set it to false if the profiler is started and ShouldProfileThread returns false for the thread.

Comment on attachment 8858651 [details] Bug 1346592 - When stopping the profiler, mark all ThreadInfos as not being profiled. https://reviewboard.mozilla.org/r/130628/#review133536 ::: tools/profiler/core/ThreadInfo.h:33 (Diff revision 1) > > mozilla::NotNull<PseudoStack*> Stack() const { return mPseudoStack; } > > - void SetHasProfile() { mHasProfile = true; } > + void StartSampling(); > + void StopSampling(); > + bool IsBeingProfiled() { return mIsBeingProfiled; } The code already uses "profiling" in some places and "sampling" in others. Nonetheless, it's probably best to avoid mixing the terms for functions as closely related as these! Use {Start,Stop}Profiling()? ::: tools/profiler/core/platform.cpp:1865 (Diff revision 1) > NotNull<PseudoStack*> pseudoStack = info->Stack(); > > tlsPseudoStack.set(pseudoStack.get()); > > if (ShouldProfileThread(aLock, info)) { > - info->SetHasProfile(); > + if (gPS->IsActive(aLock)) { We might as well merge these two conditions, and put IsActive() first now. ::: tools/profiler/core/platform.cpp:2487 (Diff revision 1) > - if (gPS->FeatureJS(aLock)) { > - PS::ThreadVector& liveThreads = gPS->LiveThreads(aLock); > + PS::ThreadVector& liveThreads = gPS->LiveThreads(aLock); > - for (uint32_t i = 0; i < liveThreads.size(); i++) { > + for (uint32_t i = 0; i < liveThreads.size(); i++) { > - ThreadInfo* info = liveThreads.at(i); > + ThreadInfo* info = liveThreads.at(i); > - if (ShouldProfileThread(aLock, info)) { > - MOZ_RELEASE_ASSERT(info->HasProfile()); > + if (info->IsBeingProfiled()) { > + if (gPS->FeatureJS(aLock)) { I recently changed these lines to fix bug 1355807. I think the other parts of this patch mean that undoing my changes from that bug is ok here, but I want to double-check that you intended this and it's not an accidental reversion due to rebasing.

Comment on attachment 8858651 [details] Bug 1346592 - When stopping the profiler, mark all ThreadInfos as not being profiled. https://reviewboard.mozilla.org/r/130628/#review133536 > I recently changed these lines to fix bug 1355807. I think the other parts of this patch mean that undoing my changes from that bug is ok here, but I want to double-check that you intended this and it's not an accidental reversion due to rebasing. Oh, interesting. I hadn't fully understood the patch from bug 1355807 when I looked at it last week, but now that I've written this patch, it makes a lot more sense. And yes, I believe the other changes from this patch should make the revert ok. In particular, the call to StopProfiling() right after this line will make sure that we won't enter this branch again for this thread unless StartProfiling has been called in the meantime, in which case we will also have called startJSSampling, which allows us to call stopJSSampling again.

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/1b277ae21ba5 When stopping the profiler, mark all ThreadInfos as not being profiled. r=njn

backed out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92208573&repo=autoland&lineNumber=5024

I caught that mistake before I landed, refreshed the patch to fix it, but then didn't push to mozreview before I clicked the autoland button. :(

Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/802a9bc40bc6 Backed out changeset 1b277ae21ba5

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/855059cc1406 When stopping the profiler, mark all ThreadInfos as not being profiled. r=njn