If RunMonitorThread's loop detects a hang in a given thread, and then that thread calls NotifyWait, it will set mWaiting to true before the RunMonitorThread loops due to the mLock.NotifyAll() call. This results in the thread being skipped, which means its mHanging status and other values are left untouched, and the original hang isn't reported until the next time NotifyActivity is called, at which point the hang is reported with a potentially significant inflation of its duration, due to the time spent waiting.

Comment on attachment 8956888 [details] Bug 1443688 - Ensure NotifyWait doesn't result in indefinite hangs https://reviewboard.mozilla.org/r/225846/#review231794 ::: toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp:241 (Diff revision 1) > > if (mWaiting) { > return; > } > > + mWaitingInitiated = true; Thanks for catching this! I think we should consider taking a different approach to this though which doesn't require the extra boolean. If we're in this awkward hanging situation when we start waiting, we can know in NotifyWait(), because mHanging will be true. I think what we should do here is do a simple check (N.B. I think you should wait for me to re-land bug 1437167 so that you can do this, as the interval type changes): Update(); if (mHanging) { // We were hanging! We're done with that now, so let's report it. // ReportHang() doesn't do much work on the current thread, and is // safe to call from any thread as long as we're holding the lock. ReportHang(mInterval - mHangStart); mHanging = false; } mWaiting = true;

SGTM

Hey Nika, are you actively working on Bug 1437167, or do you mind if I make these changes now? This is blocking information that I would like to get sooner rather than later about forcepaint hangs. It seems like the only conflict is going to be changing mInterval to mLastActivity, no?

(In reply to Doug Thayer [:dthayer] from comment #4) > Hey Nika, are you actively working on Bug 1437167, or do you mind if I make > these changes now? This is blocking information that I would like to get > sooner rather than later about forcepaint hangs. It seems like the only > conflict is going to be changing mInterval to mLastActivity, no? Please go ahead and land it. I ended up getting blocked on some platform specific compiling issues for Bug 1437167, so it probably won't land for at least a few more days. I can easily rebase my code on top of yours.

Comment on attachment 8956888 [details] Bug 1443688 - Ensure NotifyWait doesn't result in indefinite hangs https://reviewboard.mozilla.org/r/225846/#review234772 r=me if you get rid of the random data transfer hunks ::: dom/ipc/TabChild.cpp (Diff revision 2) > #include "TabParent.h" > #include "mozilla/Preferences.h" > #include "mozilla/BrowserElementParent.h" > #include "mozilla/ClearOnShutdown.h" > #include "mozilla/EventListenerManager.h" > -#include "mozilla/dom/DataTransfer.h" What is this change doing here? ::: dom/ipc/TabChild.cpp:1959 (Diff revision 2) > nsCOMPtr<nsIDragSession> dragSession = nsContentUtils::GetDragSession(); > if (dragSession) { > dragSession->SetDragAction(aDragAction); > dragSession->SetTriggeringPrincipalURISpec(aPrincipalURISpec); > - RefPtr<DataTransfer> initialDataTransfer = dragSession->GetDataTransfer(); > + nsCOMPtr<nsIDOMDataTransfer> initialDataTransfer; > + dragSession->GetDataTransfer(getter_AddRefs(initialDataTransfer)); What is this change doing here?

(In reply to Nika Layzell [:mystor] from comment #7) > What is this change doing here? > > ::: dom/ipc/TabChild.cpp:1959 > (Diff revision 2) > > nsCOMPtr<nsIDragSession> dragSession = nsContentUtils::GetDragSession(); > > if (dragSession) { > > dragSession->SetDragAction(aDragAction); > > dragSession->SetTriggeringPrincipalURISpec(aPrincipalURISpec); > > - RefPtr<DataTransfer> initialDataTransfer = dragSession->GetDataTransfer(); > > + nsCOMPtr<nsIDOMDataTransfer> initialDataTransfer; > > + dragSession->GetDataTransfer(getter_AddRefs(initialDataTransfer)); > > What is this change doing here? Huh. Uh, thank you for catching that. Honestly I have no idea where those changes came from.

Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ae5f0713084e Ensure NotifyWait doesn't result in indefinite hangs r=mystor