1788879 - Do not initialize or otherwise notify during shutdown

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jens Stutte [:jstutte]]

Based on 6da808e3-2f2a-4f0a-ad52-25f260220902 it seems that we want to initialize potentially heavy-weight COM objects while already asked to shutdown.

We can probably check here in NotificationController::WillRefresh also for ProcessChild::ExpectingShutdown() ?

Comment 1

[James Teh [:Jamie]]

Apologies for the potentially silly question, but how do you tell things are shutting down from this stack? I don't see anything about shutdown in the stack...

Comment 2

[Jens Stutte [:jstutte]]

Sorry, this deserves more context, I'll provide some on Monday.

Comment 3

[Jens Stutte [:jstutte]]

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

Sorry, this deserves more context, I'll provide some on Monday.

The context is now in bug 1789231, let me know if you still have questions.

Comment 4

[James Teh [:Jamie]]

If we put the ExpectingShutdown check where you suggest in comment 0, that's right at the start of the runnable. Wouldn't that be caught by centralised mitigations for this; e.g. high priority shutdown message, since our code has only just started? Or are we not planning to implement centralised mitigations for some reason?

I can think of places later in WillRefresh where we might want to return early if we're expecting shutdown, though. The question is just whether you really want the check right at the start or whether that will soon be made redundant.

Comment 5

[Jens Stutte [:jstutte]]

(In reply to James Teh [:Jamie] from comment #4)

If we put the ExpectingShutdown check where you suggest in comment 0, that's right at the start of the runnable. Wouldn't that be caught by centralised mitigations for this; e.g. high priority shutdown message, since our code has only just started? Or are we not planning to implement centralised mitigations for some reason?

Yes, your doubt is valid and I was too short-eyed in suggesting just that check. As bug 1755376 showed, raising priority of shutdown messages alone does not solve the problem in general. I updated the context in that sense.

I can think of places later in WillRefresh where we might want to return early if we're expecting shutdown, though. The question is just whether you really want the check right at the start or whether that will soon be made redundant.

I think later checks would be good, in particular if there are loops. However, in this case I expect there to be just a slow or blocked response from the OS in initializing the COM object, thus I am not too optimistic on additional checks. Not sure if there are situations where this involves user interaction at OS level like asking for permissions? Probably in this specific case moving the operation as such to a different thread and having a synchronization loop on the main thread could help to keep the browser responsive in general and we could abort that loop in case of shutdown (see bug 1740889 for an example) ?

Comment 6

[James Teh [:Jamie]]

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

Yes, your doubt is valid and I was too short-eyed in suggesting just that check. As bug 1755376 showed, raising priority of shutdown messages alone does not solve the problem in general.

Okay, but will we still be doing that anyway or should I add the check to the start of WillRefresh as well as later checks?

I think later checks would be good, in particular if there are loops. However, in this case I expect there to be just a slow or blocked response from the OS in initializing the COM object, thus I am not too optimistic on additional checks.

Yeah, the particular case you referenced in comment 0 is probably not something we can really fix. I don't know why that would take a long time. The good news is that we're ditching a lot of this COM stuff in the content process with the current a11y engine re-architecture (Cache the World).

Not sure if there are situations where this involves user interaction at OS level like asking for permissions?

It shouldn't.

Comment 7

[Jens Stutte [:jstutte]]

(In reply to James Teh [:Jamie] from comment #6)

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

Yes, your doubt is valid and I was too short-eyed in suggesting just that check. As bug 1755376 showed, raising priority of shutdown messages alone does not solve the problem in general.

Okay, but will we still be doing that anyway or should I add the check to the start of WillRefresh as well as later checks?

I'd not expect this to happen soon, if ever. We do not yet know if changing the order on the main thread here would be safe, and having now ProcessChild::ExpectingShutdown() to check also in critical situations that cannot be avoided even with higher priority it might not be worth doing it. However I'd add checks only where you know we have potentially longer runtime (be it from evidence in crash-stats or because you already know) and where you know you can safely interrupt an operation (loop). Before we start to add a check at the beginning of every runnable, we should probably think of a more general approach.

And sorry for my first suggestion to be kind of deviating from the core of the problem...

(If you have any good suggestion for what we can really do here, we should probably update the description accordingly, thanks)

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1788879: Return early from potentially long-running a11y tasks if a content process has been asked to shut down.

If the process is asked to shut down while we're in the middle of handling long-running tasks, we should return early to avoid pointless work and allow the process to shut down sooner.
This patch adds ExpectingShutdown checks near various things which could potentially take a while; e.g. building the local tree, serializing the IPC tree, building the IPC cache, etc.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/097fd3e9914e
Return early from potentially long-running a11y tasks if a content process has been asked to shut down. r=eeejay

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/097fd3e9914e