Bugzilla

Andrew Sutherland [:asuth] (he/him)

Assignee

Comment 2

•

3 years ago

Yeah, and deeper inside in the event processing we try to create a new service worker registrar which then fails during resurrection, despite the comment in the beginning saying, we want to avoid resurrection.

To my feeling, we should improve and use AppShutdown::IsShuttingDown() more often. I see two ToDos here:

The current implementation of AppShutdown::IsShuttingDown() relies on AppShutdown::OnShutdownConfirmed(); being called. Instead it should probably just check, if AppShutdown::GetCurrentShutdownPhase() >= ShutdownPhase::AppShutdownConfirmed.
Introduce more checks for shutdown in the following places:

generation of new global objects (like the service worker registrar here)
dispatching of particular events, throwing NS_ERROR_ILLEGAL_DURING_SHUTDOWN

A more advanced architectural change could be, to associate to each event type a latest possible shutdown phase for this event and check that directly during dispatch (with an assert in debug and an if in release). The default would be again ShutdownPhase::AppShutdownConfirmed.

Flags: needinfo?(nika)

Flags: needinfo?(bugmail)

Comment 3

•

3 years ago

I think this is https://bugzilla.mozilla.org/show_bug.cgi?id=1697972#c13 where I found that I think this set of symptoms is happening because a hidden window is being accidentally created that races xpcshell shutdown on windows and this starts up the ServiceWorkerManager. Fixing that fell off my radar but is likely the right fix to fix a whole class of this problem.

Flags: needinfo?(bugmail)

Andrew Sutherland [:asuth] (he/him)

Updated

•

3 years ago

Flags: needinfo?(nika)

Andrew Sutherland [:asuth] (he/him)

Comment 4

•

3 years ago

That said, I do agree that ServiceWorkerManager and other singleton getters should check the shutdown phase and return an error when in shutdown. And potentially we should make sure xpcshell's shutdown routine makes sure that the shutdown checks correctly indicate this.

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 9

•

3 years ago

We should trace a path forward here (at least in terms of todos/bugs).

Flags: needinfo?(jstutte)

Assignee

Updated

•

3 years ago

Depends on: 1716530

Comment hidden (Intermittent Failures Robot)

Florian Quèze [:florian]

Comment 14

•

2 years ago

The high frequency failure that started a few hours ago with the same assertion but a different stack was handled in bug 1753598.

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 22

•

2 years ago

(In reply to Intermittent Failures Robot from comment #21)

4 failures in 3497 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

autoland: 2

mozilla-central: 2

Platform and build breakdown:

windows10-32-2004-qr: 4

debug: 4

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1709184&startday=2022-07-18&endday=2022-07-24&tree=all

At least one instance here seems to be caused by a late initialization of MCSInfo. This singleton has been introduced by bug 1766307.

Flags: needinfo?(jstutte) → needinfo?(azebrowski)

Comment 23

•

2 years ago

Thanks for this, taking a look now.

Flags: needinfo?(azebrowski)

Comment hidden (Intermittent Failures Robot)

Comment 27

•

2 years ago

Attached file Bug 1709184 - Intermittent Assertion failure: false (ClearOnShutdown for phase that already was cleared), at /builds/worker/checkouts/gecko/xpcom/base/ClearOnShutdown.cpp:20 (obsolete) — Details

Remove XPCOM shutdown listener from MCSInfo to remove possibility of the listener starting after the shutdown process has already begun.

Updated

•

2 years ago

Assignee: nobody → azebrowski

Status: NEW → ASSIGNED

Comment 28

•

2 years ago

Comment on attachment 9287599 [details]
Bug 1709184 - Intermittent Assertion failure: false (ClearOnShutdown for phase that already was cleared), at /builds/worker/checkouts/gecko/xpcom/base/ClearOnShutdown.cpp:20

Beta/Release Uplift Approval Request

User impact if declined: None
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Small patch that simplifies deallocation logic + adds several additional asserts. Treeherder link showing failure reduction: https://treeherder.mozilla.org/intermittent-failures/bugdetails?startday=2022-07-17&endday=2022-07-28&tree=all&bug=1709184
String changes made/needed:
Is Android affected?: Unknown

Attachment #9287599 - Flags: approval-mozilla-beta?

Comment 29

•

2 years ago

Moved the patch over from Bug 1781259 as it fixes this issue.

Status: ASSIGNED → RESOLVED

Closed: 2 years ago

Resolution: --- → FIXED

Comment 30

•

2 years ago

Would like to update the patch's commit message after moving it over, but Phabricator isn't cooperating and I have to call it a night. I'll check back tomorrow + plan to update it to be:

Remove XPCOM shutdown listener from MCSInfo to remove possibility of the listener starting after the shutdown process has already begun.

Thanks!

Assignee

Comment 31

•

2 years ago

(In reply to az [:az] from comment #29)

Moved the patch over from Bug 1781259 as it fixes this issue.

As shown by the past, there might be different causes for this assertion, not just one (though this is probably the most frequent currently). I'd prefer in these cases to create new bugs for specific fixes and make this bug depend on them.

Updated

•

2 years ago

Depends on: 1782313

Comment 32

•

2 years ago

•

Edited

Reopening as this will be fixed in Bug 1782313

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Comment hidden (Intermittent Failures Robot)

Dianna Smith [:diannaS]

Comment 34

•

2 years ago

:az Is the beta uplift still needed?

Flags: needinfo?(azebrowski)

Comment 35

•

2 years ago

Comment on attachment 9287599 [details]
Bug 1709184 - Intermittent Assertion failure: false (ClearOnShutdown for phase that already was cleared), at /builds/worker/checkouts/gecko/xpcom/base/ClearOnShutdown.cpp:20

Revision D152954 was moved to bug 1782313. Setting attachment 9287599 [details] to obsolete.

Attachment #9287599 - Attachment is obsolete: true

Attachment #9287599 - Flags: approval-mozilla-beta?

Comment hidden (Intermittent Failures Robot)

Comment 37

•

2 years ago

•

Edited

.

Assignee: azebrowski → nobody

Flags: needinfo?(azebrowski)

Comment hidden (Intermittent Failures Robot)

Cosmin Sabou [:CosminS]

Comment 41

•

2 years ago

These stopped failing beginning with this merge https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=cbd753d186199d816e1d097631573f601932b96e
TH link.

Whiteboard: [stockwell disable-recommended] → [stockwell needswork:owner]

Comment hidden (Intermittent Failures Robot)

Cosmin Sabou [:CosminS]

Updated

•

2 years ago

Whiteboard: [stockwell disable-recommended]

Comment hidden (Intermittent Failures Robot)

Assignee

Comment 45

•

2 years ago

There seem to be some cases where we dispatch a RunOnShutdown to the main thread well before we have entered shutdown but end up executing that runnable only after we reached XPCOMShutdown.

We can either check inside the lambda of the "MCSInfo::MCSInfo" runnable if we are in shutdown (and just do the deletion in case) or we might want to check if this assert is actually needed or if the release behavior isn't always the right behavior. Or we want to have a thread-safe InsertIntoShutdownList such that we can avoid the thread hopping just for inserting?

Flags: needinfo?(kmaglione+bmo)

Kris Maglione [:kmag]

Comment 46

•

2 years ago

(In reply to Jens Stutte [:jstutte] from comment #45)

There seem to be some cases where we dispatch a RunOnShutdown to the main thread well before we have entered shutdown but end up executing that runnable only after we reached XPCOMShutdown.

We can either check inside the lambda of the "MCSInfo::MCSInfo" runnable if we are in shutdown (and just do the deletion in case) or we might want to check if this assert is actually needed or if the release behavior isn't always the right behavior. Or we want to have a thread-safe InsertIntoShutdownList such that we can avoid the thread hopping just for inserting?

The assertion is needed, unfortunately. Without it, we could run into all sorts of undefined behavior when things run too late in shutdown. The most common manifestation, though, is that someone calls a getter that calls ClearOnShutdown after the target shutdown phase, ClearOnShutdown immediately clears the target, the getter returns null, and we then null pointer crash instead of asserting. The assertion is mainly there to make it clearer where the failure really is, rather than trying to work backward from a null pointer crash.

For this particular case, I think the solution may be to call NS_ProcessPendingEvents before advancing to the XPCOMShutdown phase like we do before progressing to later shutdown phases. That way any runnable dispatched before XPCOMShutdown will be guaranteed to run in time to safely register its shutdown observers.

That said... Are you sure that runnable is actually guaranteed to be dispatched before shutdown has started? Because, without a shutdown blocker, you generally can't guarantee that for background thread code. And I don't see a shutdown blocker anywhere in that code.

Flags: needinfo?(kmaglione+bmo)

Assignee

Comment 47

•

2 years ago

•

Edited

(In reply to Kris Maglione [:kmag] from comment #46)

The assertion is needed, unfortunately. Without it, we could run into all sorts of undefined behavior when things run too late in shutdown. The most common manifestation, though, is that someone calls a getter that calls ClearOnShutdown after the target shutdown phase, ClearOnShutdown immediately clears the target, the getter returns null, and we then null pointer crash instead of asserting. The assertion is mainly there to make it clearer where the failure really is, rather than trying to work backward from a null pointer crash.

It seems to me that in release builds InsertIntoShutdownList (used by RunOnShutdown) handles this by just calling aObserver->Shutdown(); immediately. Obviously this can still have any sorts of unexpected side-effects, so I think the intention of that assert is generally right.

For this particular case, I think the solution may be to call NS_ProcessPendingEvents before advancing to the XPCOMShutdown phase like we do before progressing to later shutdown phases. That way any runnable dispatched before XPCOMShutdown will be guaranteed to run in time to safely register its shutdown observers.

Thanks, that seems like something we should do, yes. Probably also before XPCOMWillShutdown.

That said... Are you sure that runnable is actually guaranteed to be dispatched before shutdown has started? Because, without a shutdown blocker, you generally can't guarantee that for background thread code. And I don't see a shutdown blocker anywhere in that code.

At least MCSInfo::GetInstance does check AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed) at its beginning, but as we seem to be off-main-thread here that can race right afterwards with whatever is happening on the main thread? This would be probably a candidate singleton for something like bug 1796566 (and gives me some input for the thread safety questions I have there). Instantiating singletons and thus adding a blocker for them should be protected by a mutex that prevents us from advancing shutdown phases in parallel in such situations, I think. But in the meantime let's try with processing the pending events before each AdvanceShutdownPhase to improve the situation here (and potentially for other off-main-thread instantiated singletons we already have and that follow the same pattern).

Assignee

Comment 48

•

2 years ago

Attached file Bug 1709184 - Part 1: Process pending MT events before advancing the shutdown phase. r?#xpcom-reviewers — Details

Updated

•

2 years ago

Assignee: nobody → jstutte

Assignee

Comment 49

•

2 years ago

Attached file Bug 1709184 - Part 2: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r?#xpcom-reviewers — Details

Depends on D160176

Kris Maglione [:kmag]

Comment 50

•

2 years ago

(In reply to Jens Stutte [:jstutte] from comment #47)

It seems to me that in release builds InsertIntoShutdownList (used by RunOnShutdown) handles this by just calling aObserver->Shutdown(); immediately. Obviously this can still have any sorts of unexpected side-effects, so I think the intention of that assert is generally right.

Yes, that's what I mean. The typical pattern is:

static StaticRefPtr<Thing> sThing;

if (!sThing) {
  sThing = new Thing();
  ClearOnShutdown(&sThing);
}
return sThing;

Then the shutdown callback runs immediately, the function returns null, and the caller crashes on the null deref. But there are often worse problems from just calling the constructor during shutdown, since it often winds up reinitializing things which have already been shutdown, or calling into code which isn't designed to work during shutdown.

That said... Are you sure that runnable is actually guaranteed to be dispatched before shutdown has started? Because, without a shutdown blocker, you generally can't guarantee that for background thread code. And I don't see a shutdown blocker anywhere in that code.

At least MCSInfo::GetInstance does check AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed) at its beginning, but as we seem to be off-main-thread here that can race right afterwards with whatever is happening on the main thread?

Yeah, technically that could race, but I guess it's unlikely enough that it probably isn't especially relevant to the assertion. Processing the pending events is likely enough to at least make the intermittent go away.

Assignee

Updated

•

2 years ago

Updated

•

2 years ago

Attachment #9300060 - Attachment description: Bug 1709184 - Process pending MT events before advancing the shutdown phase. r?#xpcom-reviewers → Bug 1709184 - Part 10: Process pending MT events before advancing the shutdown phase. r?#xpcom-reviewers

Updated

•

2 years ago

Attachment #9300175 - Attachment description: Bug 1709184 - Add a paranoia check against re-entrance in AdvanceShutdownPhase. r?#xpcom-reviewers → Bug 1709184 - Part 11: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r?#xpcom-reviewers

Comment hidden (Intermittent Failures Robot)

Natalia Csoregi [:nataliaCs]

Assignee

Updated

•

2 years ago

Comment 52

•

1 year ago

Pushed by jstutte@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac48b4d61b39
Part 10: Process pending MT events before advancing the shutdown phase. r=xpcom-reviewers,nika,kmag
https://hg.mozilla.org/integration/autoland/rev/46d7864a45df
Part 11: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r=xpcom-reviewers,nika

Comment 53

•

1 year ago

Backed out for xpcshell failures.

Flags: needinfo?(jstutte)

Assignee

Comment 54

•

1 year ago

•

Edited

(In reply to Natalia Csoregi [:nataliaCs] from comment #53)

* [SUMMARY: AddressSanitizer: SEGV /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:53:12 in nsAutoOwningEventTarget](https://treeherder.mozilla.org/logviewer?job_id=396846889&repo=autoland&lineNumber=3527)

This one is interesting. IIUC we want to shut down the component manager and in doing so we want to create an AutoSafeJSContext that ends up trying to assign something to mUnprivilegedJunkScope.

This wants to create a WeakReference and the constructor does a MOZ_WEAKPTR_INIT_THREAD_SAFETY_CHECK which wants to create a nsAutoOwningEventTarget that will find GetCurrentSerialEventTarget() being nullptr.

In terms what the patch might have changed here: Indeed we removed this NS_ProcessPendingEvents(thread); which inside AdvanceShutdownPhase is only executed, if we had to notify our observers - which is unlikely here, as we just killed the observer service a few lines above.

This would be repaired by D160628, I think, so landing the entire stack might be the better call here.

Flags: needinfo?(jstutte)

Comment 55

•

1 year ago

Pushed by jstutte@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1181bdcde598
Part 10: Process pending MT events before advancing the shutdown phase. r=xpcom-reviewers,nika,kmag
https://hg.mozilla.org/integration/autoland/rev/a4c2b2d0b63a
Part 11: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r=xpcom-reviewers,nika

Marian-Vasile Laza

Comment 56

•

1 year ago

•

Edited

Backed out 3 changesets (Bug 1768581, Bug 1709184) for causing multiple failures on LateWriteChecks.cpp.
Backout link
Push with failures <--> gpu
Failure Log
Also R1 Failure Log, Also R2 Failure Log, ...

Also X4 almost perma --> Failure Log

Flags: needinfo?(jstutte)

Assignee

Comment 57

•

1 year ago

Wow.

Flags: needinfo?(jstutte)

Comment 58

•

1 year ago

Pushed by jstutte@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/032bf39acabb
Part 10: Process pending MT events before advancing the shutdown phase. r=xpcom-reviewers,nika,kmag
https://hg.mozilla.org/integration/autoland/rev/c7465ec6154e
Part 11: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r=xpcom-reviewers,nika

Cristian Tuns

Comment 59

•

1 year ago

•

Edited

Backed out for causing xpcshell failures

Backout link
Push with failures
Failure Log
Failure line: TEST-UNEXPECTED-FAIL | extensions/pref/autoconfig/test/unit/test_autoconfig.js | xpcshell return code: -6
SUMMARY: ThreadSanitizer: SEGV /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:53:12 in nsAutoOwningEventTarget
TEST-UNEXPECTED-FAIL | extensions/pref/autoconfig/test/unit/test_autoconfig_nonascii.js | xpcshell return code: -6
SUMMARY: ThreadSanitizer: SEGV /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:53:12 in nsAutoOwningEventTarget
TEST-UNEXPECTED-FAIL | extensions/pref/autoconfig/test/unit/test_autoconfig_no_sandbox.js | xpcshell return code: -6
SUMMARY: ThreadSanitizer: SEGV /builds/worker/checkouts/gecko/xpcom/base/nsISupportsImpl.cpp:53:12 in nsAutoOwningEventTarget

Can you also please take a look at this? https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&searchStr=android%2Cbrowsertime&revision=bd3bc1478107b9a72ce5dded92b7821e87a0e052&selectedTaskRun=TQNO9jTxSQ2KK0ouVXURHA.0

Flags: needinfo?(jstutte)

Comment 60

•

1 year ago

Pushed by jstutte@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98f1dc6f0123
Part 10: Process pending MT events before advancing the shutdown phase. r=xpcom-reviewers,nika,kmag
https://hg.mozilla.org/integration/autoland/rev/a3d89b2ef44a
Part 11: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r=xpcom-reviewers,nika

Updated

•

1 year ago

Attachment #9300060 - Attachment description: Bug 1709184 - Part 10: Process pending MT events before advancing the shutdown phase. r?#xpcom-reviewers → Bug 1709184 - Part 1: Process pending MT events before advancing the shutdown phase. r?#xpcom-reviewers

Updated

•

1 year ago

Attachment #9300175 - Attachment description: Bug 1709184 - Part 11: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r?#xpcom-reviewers → Bug 1709184 - Part 2: Add a paranoia check against re-entrance in AdvanceShutdownPhase. r?#xpcom-reviewers

Assignee

Updated

•

1 year ago

Flags: needinfo?(jstutte)