Closed Bug 1437575 Opened 2 years ago Closed 2 years ago

Better shutdown hang reporting: all the notifications must be received

Categories

(Core :: DOM: Core & HTML, defect, P2)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Depends on 4 open bugs, Blocks 8 open bugs)

Details

Crash Data

Attachments

(1 file, 1 obsolete file)

When firefox shutdown hangs, it's often because some component blocks the main-thread and doesn't not allow the dispatching of the shutdown notifications. Here some bugs: 1405290, 1435958, 1435960, 1435961, 1435962, 1435966, 1435963, 1435964.

I want to improve the logging to know when this happens.
Attached patch hang.patch (obsolete) — Splinter Review
Attachment #8950265 - Flags: review?(bugs)
Attached patch hang.patchSplinter Review
Attachment #8950265 - Attachment is obsolete: true
Attachment #8950265 - Flags: review?(bugs)
Attachment #8950266 - Flags: review?(bugs)
Comment on attachment 8950266 [details] [diff] [review]
hang.patch

>+
>+bool sShutdownCompleted = false;
This is read on watchdog thread, but written in the main thread, yet the variable isn't atomic.
what guarantees synchronization? Use Atomic.


>+XPCOMShutdownCompleted()
>+{
>+  MOZ_DIAGNOSTIC_ASSERT(sShutdownCompleted == false);
>+  sShutdownCompleted = true;
sShutdownNotified


>@@ -855,16 +856,18 @@ ShutdownXPCOM(nsIServiceManager* aServMg
> 
>       nsCOMPtr<nsIServiceManager> mgr;
>       rv = NS_GetServiceManager(getter_AddRefs(mgr));
>       if (NS_SUCCEEDED(rv)) {
>         mozilla::KillClearOnShutdown(ShutdownPhase::Shutdown);
>         observerService->NotifyObservers(mgr, NS_XPCOM_SHUTDOWN_OBSERVER_ID,
>                                          nullptr);
>       }
>+
>+      mozilla::XPCOMShutdownCompleted();
Perhaps call this XPCOMShutdownNotified, since xpcom isn't really shutdown yet at this point
Attachment #8950266 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c669d852f87
Better shutdown hang reporting: all the notifications must be received, r=smaug
Blocks: 1435343
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/9c669d852f87
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Want to request uplift here to beta to help potentially diagnose things for bug 1405290?
Flags: needinfo?(amarchesini)
Comment on attachment 8950266 [details] [diff] [review]
hang.patch

Approval Request Comment
[Feature/Bug causing the regression]: Shutdown hanging crash report
[User impact if declined]: crash report doesn't say why hanging happens and wrongly shows a worker hanging all the time.
[Is this code covered by automated tests?]: n/a
[Has the fix been verified in Nightly?]:n/a
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: This patch introduces a check about the receiving of all the shutdown notifications. In case some of them are not received, after X seconds, firefox crashes.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8950266 - Flags: approval-mozilla-beta?
Comment on attachment 8950266 [details] [diff] [review]
hang.patch

Diagnostic patch for shutdown crashes, for beta 14.
Attachment #8950266 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1405290
No longer depends on: 1435958
Crash Signature: [@ shutdownhang | mozilla::dom::workers::RuntimeService::Cleanup] [@ shutdownhang | __psynch_cvwait | <name omitted> | mozilla::dom::workers::RuntimeService::Cleanup ] [@ shutdownhang | ZwWaitForAlertByThreadId | mozilla::dom::workers::RuntimeService::C…
Depends on: 1434618, 1422036
Depends on: 1356853
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.