Bug 1588152 Comment 50 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

With many, many, MANY thanks to :whimboo providing the pernosco trace https://pernos.co/debug/Uq1o5n_s5wLvmwUfY60jHg/index.html in https://bugzilla.mozilla.org/show_bug.cgi?id=1651542#c91 it appears that the situation here is deadlock between our use an nsIAsyncShutdownClient on "profile-change-teardown" and our use of a normal observer on "profile-change-teardown".  If the async shutdown service gets notified first, it results in deadlock.

The basic scenario is:
- For parent intercept, we wanted to change the point at which we started shutdown of ServiceWorkerManager via MaybeStartShutdown.  Originally, this was xpcom-shutdown but this was way too late for parent-intercept mode with remote workers.  So we moved it up to "profile-change-teardown", although I think there was some fuzziness around which observer was most optimal.  The observer was a pre-existing mechanism that we were reusing and would leave using xpcom-shutdown in non-parent-intercept.
- We also added a shutdown blocker because that's the right mechanism.
- https://phabricator.services.mozilla.com/D26166#1213324 is my restating statement on this situation.

At a meta level, I think the general situation was that we were concerned about changing the child intercept behavior and although both of these changes landed in the same patch, the evolution of the implementation obscured things somewhat, with the `GetStartShutdownTopic()` indirection obscuring that we'd landed on using the same observer notification for both.  (I vaguely recall in a longer video discussion having them be different phases but then reality aligned them to be the same, but no red flags went up.)

From a fix perspective, we no longer support child intercept so it's straightforward for us to eliminate the use of observer notifications entirely.
With many, many, MANY thanks to :whimboo providing the pernosco trace https://pernos.co/debug/Uq1o5n_s5wLvmwUfY60jHg/index.html in https://bugzilla.mozilla.org/show_bug.cgi?id=1651542#c91 it appears that the situation here is deadlock between our use of an nsIAsyncShutdownClient on "profile-change-teardown" and our (dynamically arrived at) use of a normal observer on "profile-change-teardown".  If the async shutdown service gets notified first, it results in deadlock.

The basic scenario is:
- For parent intercept, we wanted to change the point at which we started shutdown of ServiceWorkerManager via MaybeStartShutdown.  Originally, this was xpcom-shutdown but this was way too late for parent-intercept mode with remote workers.  So we moved it up to "profile-change-teardown", although I think there was some fuzziness around which observer was most optimal.  The observer was a pre-existing mechanism that we were reusing and would leave using xpcom-shutdown in non-parent-intercept.
- We also added a shutdown blocker because that's the right mechanism.
- https://phabricator.services.mozilla.com/D26166#1213324 is my restating statement on this situation.

At a meta level, I think the general situation was that we were concerned about changing the child intercept behavior and although both of these changes landed in the same patch, the evolution of the implementation obscured things somewhat, with the `GetStartShutdownTopic()` indirection obscuring that we'd landed on using the same observer notification for both.  (I vaguely recall in a longer video discussion having them be different phases but then reality aligned them to be the same, but no red flags went up.)

From a fix perspective, we no longer support child intercept so it's straightforward for us to eliminate the use of observer notifications entirely.

Back to Bug 1588152 Comment 50