In bug 1303077, I added an annotation, "ForcePaintInProgress", for any hangs that occur after a paint is forced. The mechanism isn't perfect though. The BackgroundHangMonitor that we're annotating cares about when we stop and start processing events. For getting rid of the spinner in bug 1300411 (which all of this is about), what we care most about are hangs that occur between the point where we ask the ProcessHangMonitor to force paint, and when the paint actually occurs. I talked to billm about this, and what we want to do is create a BackgroundHangMonitor that has its NotifyActivity called when a force paint is requested, and NotifyWait called when the paint is completed. The BackgroundHangThread will poll at interval during that window if it exceeds a certain threshold (I guess we'll pick some numbers... maybe 128ms). What's particularly nice about this is that we will then have a special named "thread" inside threadHangStats that will just contain hangs that occurred during tab switch, and nothing else. No filtering necessary. Reading the documentation at http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/xpcom/threads/BackgroundHangMonitor.h#28, I think this should be possible.

Note that if we go for this, we can safely back out bug 1303077.

Hey jimchen, do you know if what billm and I are looking for is possible? We're basically looking for a dedicated BackgroundHangMonitor for the main thread in the content process that starts monitoring for hangs once force paint is requested, and until the paint is completed. This is in tandem to the already-existing BackgroundHangMonitor that's running on the main thread in the content process, monitoring events. Note that the notification to force paint comes in off of the main thread, but the notification that the paint has occurred occurs on the main thread. Before I start really diving into this, is what we're looking for even possible / advisable with our current BHR infrastructure?

I think you can get away with creating a special BackgroundHangMonitor/BackgroundHangThread pair for forced paints. Basically, create a special BHT on the main thread, and use it to initialize a new BHM. To avoid clashing with the normal BHT, you'll need to set the mThreadID to null and bypass the TLS (coincidentally there's a bug now that makes BHT never use the TLS, but we should fix that bug). To BackgroundHangManager, it would look like a separate thread and it won't interfere with the normal BHT associated with the main thread. That new BHM would let you call NotifyActivity/NotifyWait on it, but you'll need to add synchronization because you're calling from two threads.

Comment on attachment 8802279 [details] Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread. https://reviewboard.mozilla.org/r/86680/#review85660 Sorry for the drive-by, but I was reading this over out of curiousity. I think you might want to modify FindThread to skip over private threads. ::: xpcom/threads/BackgroundHangMonitor.cpp:600 (Diff revision 1) > - uint32_t aMaxTimeoutMs) > - : mThread(BackgroundHangThread::FindThread()) > + uint32_t aMaxTimeoutMs, > + bool aUsePrivateThread) > { > #ifdef MOZ_ENABLE_BACKGROUND_HANG_MONITOR > - if (!BackgroundHangManager::sDisabled && !mThread) { > - mThread = new BackgroundHangThread(aName, aTimeoutMs, aMaxTimeoutMs); > + if (!BackgroundHangManager::sDisabled) { > + mThread = BackgroundHangThread::FindThread(); Probably want to avoid FindThread if aUsePrivateThread. It's just wasted time.

Comment on attachment 8802280 [details] Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints. https://reviewboard.mozilla.org/r/86682/#review85662 I'm not too happy with the locking here. The locking should happen inside BHR. I think it would be better if BackgroundHangThread::NotifyActivity/NotifyWait took manager->mLock. You'll have to shift around the code in NotifyWait to make sure it only takes the lock once. It looks like the original code tried to avoid taking a lock, but that seems silly to me. Taking an uncontended lock is very fast, and we're only doing this twice per event. Long term, we should also look into changing this code so it doesn't use PR_Interrupt. It would be better to notify the mLock monitor. ::: dom/ipc/ProcessHangMonitor.cpp:119 (Diff revision 1) > > private: > void ShutdownOnThread(); > > static Atomic<HangMonitorChild*> sInstance; > + BackgroundHangMonitor* mForcePaintMonitor; Please use UniquePtr here. ::: dom/ipc/ProcessHangMonitor.cpp:286 (Diff revision 1) > mContext = danger::GetJSContext(); > + mForcePaintMonitor = > + new mozilla::BackgroundHangMonitor("Gecko_Child_ForcePaint", > + 128, /* ms timeout for microhangs */ > + 4096 /* ms timeout for permahangs */, > + true /* use private thread */); Might be good to have an enum for this arg so we pass in something more informative like USE_PRIVATE_THREAD.

Comment on attachment 8802280 [details] Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints. https://reviewboard.mozilla.org/r/86682/#review86044 Looks great. Thanks. ::: dom/ipc/ProcessHangMonitor.cpp:285 (Diff revision 2) > MOZ_RELEASE_ASSERT(NS_IsMainThread()); > mContext = danger::GetJSContext(); > + mForcePaintMonitor = > + MakeUnique<mozilla::BackgroundHangMonitor>("Gecko_Child_ForcePaint", > + 128, /* ms timeout for microhangs */ > + 4096 /* ms timeout for permahangs */, Let's make the permahang time a little longer. Maybe 8 seconds? I think we cap the hang duration at that value, so I would like a little more resolution.

Comment on attachment 8802279 [details] Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread. https://reviewboard.mozilla.org/r/86680/#review86052 r+ with comments addressed. ::: xpcom/threads/BackgroundHangMonitor.h:119 (Diff revision 2) > static bool ShouldDisableOnBeta(const nsCString &); > static bool DisableOnBeta(); > > public: > static const uint32_t kNoTimeout = 0; > + enum ThreadType { Nit: add some comments ::: xpcom/threads/BackgroundHangMonitor.h:187 (Diff revision 2) > + * @param aUsePrivateThread (optional) True if a dedicated > + * BackgroundHangThread should be created for this monitor. Otherwise, > + * BackgroundHangMonitors will share the same BackgroundHangThread > + * per monitored thread. Nit: update the comment. ::: xpcom/threads/BackgroundHangMonitor.cpp:190 (Diff revision 2) > // Report a hang; aManager->mLock IS locked > Telemetry::HangHistogram& ReportHang(PRIntervalTime aHangTime); > // Report a permanent hang; aManager->mLock IS locked > void ReportPermaHang(); > // Called by BackgroundHangMonitor::NotifyActivity > void NotifyActivity(); Inline `NotifyActivity()` within the class (similar to `NotifyWait()`) now that you moved its code to `Update()`. ::: xpcom/threads/BackgroundHangMonitor.cpp:198 (Diff revision 2) > { > - NotifyActivity(); > + MonitorAutoLock autoLock(mManager->mLock); > + Update(); > mWaiting = true; > } > + void Update(); Make `Update()` private please. ::: xpcom/threads/BackgroundHangMonitor.cpp:376 (Diff revision 2) > , mHanging(false) > , mWaiting(true) > + , mThreadType(aThreadType) > , mStats(aName) > { > - if (sTlsKeyInitialized) { > + if (sTlsKeyInitialized && IsShared()) { Can you also fix the TLS bug? Right now TLS is never used because `sTlsKeyInitialized` is never set. ::: xpcom/threads/BackgroundHangMonitor.cpp:621 (Diff revision 2) > + }); > + > + if (aThreadType == THREAD_SHARED) { > + mThread = BackgroundHangThread::FindThread(); > + if (mThread) { > + threadCreationGuard.release(); This is a bit of a brain twister. Why not just say `mThread(aThreadType == THREAD_SHARED ? BackgroundHangThread::FindThread() : nullptr)` in the initializer list? Note that `mThread` is left uninitialized if `BackgroundHangManager::sDisabled` is true.

Ah I was looking at an old rev. You did change the comments. Also, billm is right that you should change FindThread() to not look at private threads.

Comment on attachment 8802280 [details] Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints. https://reviewboard.mozilla.org/r/86682/#review86058

Comment on attachment 8802279 [details] Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread. https://reviewboard.mozilla.org/r/86680/#review86052 > This is a bit of a brain twister. Why not just say `mThread(aThreadType == THREAD_SHARED ? BackgroundHangThread::FindThread() : nullptr)` in the initializer list? Note that `mThread` is left uninitialized if `BackgroundHangManager::sDisabled` is true. Oh wow, yeah, that's way simpler. Good suggestion, thanks!

Thanks for the super fast reviews, jchen and billm. jchen - I've got one final iteration up. I know I've got carry-forward powers, so it's still r+'d and landable, but I think I'd feel more confident that I interpreted your suggestions correctly if you gave it one more look. Latest iteration look okay?

Comment on attachment 8802279 [details] Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread. https://reviewboard.mozilla.org/r/86680/#review86082 ::: xpcom/threads/BackgroundHangMonitor.h:121 (Diff revision 4) > > public: > static const uint32_t kNoTimeout = 0; > + enum ThreadType { > + // For a new BackgroundHangMonitor for thread T, only create a new > + // monitoring thread for T if onen doesn't already exist. If one does, one is misspelled as "onen" ::: xpcom/threads/BackgroundHangMonitor.cpp:524 (Diff revision 4) > PRThread* threadID = PR_GetCurrentThread(); > // Lock thread list for traversal > MonitorAutoLock autoLock(manager->mLock); > for (BackgroundHangThread* thread = manager->mHangThreads.getFirst(); > thread; thread = thread->getNext()) { > - if (thread->mThreadID == threadID) { > + if (thread->mThreadID == threadID && !thread->IsShared()) { I think you meant `&& thread->IsShared()` (i.e. return a thread if it's _not_ private)

Looks great otherwise. Thanks!

Comment on attachment 8802279 [details] Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread. https://reviewboard.mozilla.org/r/86680/#review86082 > I think you meant `&& thread->IsShared()` (i.e. return a thread if it's _not_ private) Good eye!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1d7f6e18f9434c4be195a986571cc83e21c9dd Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread. r=jchen https://hg.mozilla.org/integration/mozilla-inbound/rev/48071b5d0cb211139be1ce7af508861cc79ac442 Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints. r=jchen,billm

https://hg.mozilla.org/mozilla-central/rev/1a1d7f6e18f9 https://hg.mozilla.org/mozilla-central/rev/48071b5d0cb2

I realized I missed a problem here. If we get a force paint message, there's no guarantee that JS is actually running. The next paint we do could be through our normal event loop. We should move the NotifyWait() call in RecvSetDocShellIsActive I think, although we'll need to be careful to run it only if there was a previous NotifyActive(). Sorry about this Mike.