1437575 - Better shutdown hang reporting: all the notifications must be received

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: hang.patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: hang.patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c669d852f87
Better shutdown hang reporting: all the notifications must be received, r=smaug

Comment 5

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9c669d852f87

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to request uplift here to beta to help potentially diagnose things for bug 1405290?

Comment 7

[Andrea Marchesini [:baku]]

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

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8950266 [details] [diff] [review]
hang.patch

Diagnostic patch for shutdown crashes, for beta 14.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4a24da7b8cd5