1543030 - Consider changing the way how we call AbortOperationsForProcess

Status: RESOLVED WONTFIX

(Core :: Storage: Quota Manager, task, P3)

Description

[Jan Varga [:janv]]

There are two bugs which are related, bug 1542706 and bug 1542675.
Both needs to be fixed (although it's probably the same issue), but we should consider changing AbortOperationsForProcess to be called differently.

The stack trace and especially treeherder log shows that we got AbortOperationsForProcess call on the PBackground thread, but the channel was already closed, so many IPC messages triggered by AbortOperationsForProcess fail to send. For example SendRequestAllowToClose. Our code should handle that to have protection for an unexpected channel close, but in normal situations we should try to call stuff in order.
I think, there's currently a race in normal content parent shutdown procedure between channel closing and RecvAbortOperationsForProcess.

QuotaManagerService::AbortOperationsForProcess is currently called here:
https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/dom/ipc/ContentParent.cpp#1385

Close() is called few lines below. That closes the IPC channel. The close method actually sends a message to the child and blocks the thread until the state is changed to closed.

So if we synchronously call AbortOperationsForProcess it should work better I guess.

The problem is that I'm not sure if there are separate channels for PContent and PBackground. If there are separate channels then sync AbortOperationsForProcess wouldn't help.
There's another possibility that the channel for PBackground is closed somewhere else.

We should definitely investigate this more.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

There are separate channels. In non-debug builds, PBackground will never get shutdown cleanly because the content process will self-terminate for speed so only OnChannelError shutdown will happen. In debug builds, the child process goes through full XPCOM shutdown. I have some notes at https://github.com/asutherland/asuth-gecko-notes/blob/master/ipc/SHUTDOWN.md that are not perfect but may be helpful or at least confirm your own code-reading.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:overholt, could you have a look please?

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1543030 - Consider changing the way how we call AbortOperationsForProcess; r=asuth

Comment 4

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #1)

There are separate channels. In non-debug builds, PBackground will never get shutdown cleanly because the content process will self-terminate for speed so only OnChannelError shutdown will happen. In debug builds, the child process goes through full XPCOM shutdown. I have some notes at https://github.com/asutherland/asuth-gecko-notes/blob/master/ipc/SHUTDOWN.md that are not perfect but may be helpful or at least confirm your own code-reading.

Hm, that's really good description of browser shutdown and especially content shutdown.
Anyway, what do you think about the patch I just submitted ?
Can we call AbortOperationsForProcess in the first turn of ShutDownProcess ?
Quota manager would have a better chance to abort stuff this way.

I tested the patch on try server, looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e89baae86dd17f23d1764499177255d9cf826bf7

Comment 5

[Jens Stutte [:jstutte]]

:janv, do you still think, this helps? Then assign it to dom-workers-and-storage-reviewers to be reviewed. Thank you!

Comment 6

[Jan Varga [:janv]]

We don't this for now. We may actually remove this call in future.