1727243 - Harden ServiceWorkerManager::GetInstance() against resurrection

Status: RESOLVED DUPLICATE of bug 1716530

(Core :: DOM: Service Workers, defect)

Description

[Jens Stutte [:jstutte]]

From bug 1697972 comment 44:

So looking at another random instance, I see in the log:

[task 2021-08-23T23:40:05.431Z] 23:40:05     INFO -  PID 12664 | JavaScript error: resource://gre/modules/AsyncShutdown.jsm, line 718: Error: Phase "profile-change-teardown" is finished, it is too late to register completion condition "ServiceWorkerShutdownBlocker: shutting down Service Workers"
[task 2021-08-23T23:40:05.431Z] 23:40:05     INFO -  PID 12664 | [Parent 12664, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerShutdownBlocker.cpp:108
[task 2021-08-23T23:40:05.431Z] 23:40:05     INFO -  PID 12664 | Assertion failure: mShutdownBlocker, at /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerManager.cpp:467
[task 2021-08-23T23:40:05.431Z] 23:40:05     INFO -  Initializing stack-fixing for the first stack frame, this may take a while...
[task 2021-08-23T23:40:15.303Z] 23:40:15     INFO -  PID 12664 | #01: mozilla::dom::ServiceWorkerManager::Init(mozilla::dom::ServiceWorkerRegistrar*) [dom/serviceworkers/ServiceWorkerManager.cpp:467]
[task 2021-08-23T23:40:15.305Z] 23:40:15     INFO -  PID 12664 | #02: static mozilla::dom::ServiceWorkerManager::GetInstance() [dom/serviceworkers/ServiceWorkerManager.cpp:1397]
[task 2021-08-23T23:40:15.305Z] 23:40:15     INFO -  PID 12664 | #03: mozilla::dom::Document::DispatchContentLoadedEvents() [dom/base/Document.cpp:7887]
[task 2021-08-23T23:40:15.306Z] 23:40:15     INFO -  PID 12664 | #04: mozilla::detail::RunnableMethodImpl<mozilla::dom::Document *,void (mozilla::dom::Document::*)(),1,mozilla::RunnableKind::Standard>::Run() [xpcom/threads/nsThreadUtils.h:1204]
[task 2021-08-23T23:40:15.306Z] 23:40:15     INFO -  PID 12664 | #05: mozilla::RunnableTask::Run() [xpcom/threads/TaskController.cpp:503]
[task 2021-08-23T23:40:15.307Z] 23:40:15     INFO -  PID 12664 | #06: mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex &> const&) [xpcom/threads/TaskController.cpp:805]

IIUC, we try to register a new shutdown blocker even after we already initiated shutdown. It seems, ServiceWorkerShutdownBlocker::CreateAndRegisterOn returns nullptr in this case (and produces the error from AsyncShutdown.jsm in the log we see) and we then assert on it but do not bail out in release builds.

Thus it seems that ServiceWorkerManager::GetInstance() is not protected against resurrection during shutdown and there is potential for this to happen ?

There should be a point in time during shutdown after which we do not allow new instances to be (attempted to be) created at all.

Comment 1

[Intermittent Failures Robot]

1 failures in 2580 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 1

Platform and build breakdown:

• windows10-32-qr: 1
  • debug: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1727243&startday=2021-08-23&endday=2021-08-29&tree=all

Comment 2

[Jens Stutte [:jstutte]]

So addBlocker gives us a nice error result (which could be even nicer and more specific like a NS_ERROR_ILLEGAL_DURING_SHUTDOWN), but we suppress it in CreateAndRegisterOn returning just a null pointer that we then just MOZ_ASSERT. A more explicit error handling (like a Result<already_AddRefed<ServiceWorkerShutdownBlocker>, nsresult>) as return value of CreateAndRegisterOn might have prevented this.

But the right fix is probably to not even try to create a new ServiceWorkerManager here if we are in shutdown.

Comment 3

[Jens Stutte [:jstutte]]

And reading well:

  // Note: We don't simply check gInstance for null-ness here, since otherwise
  // this can resurrect the ServiceWorkerManager pretty late during shutdown.
  static bool firstTime = true;
  if (firstTime) {

this code is not actually doing what the comment claims if we never started any service worker before shutdown. So if the only reason for this firstTime bool is to prevent resurrection, we probably should instead explicitly check if we are in shutdown here.

I can imagine this scenario to happen if a starting Firefox with some pinned tab (that wants to use service workers) is shut down before the start really finished.