1689462 - Don't run permitUnload on browser elements that we know are running in unresponsive content processes

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task)

Description

[Mike Conley (:mconley) (:⚙️)]

We currently wait 1 second for the permitUnload roundtrip to timeout before we go ahead and close a tab. I'm reasonably certain we don't check to see if the parent process is already aware that the tab(s) belong to unresponsive processes.

If we know that to be true, we should probably just skip the permitUnload step and close the tab immediately.

Comment 1

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #0)

We currently wait 1 second for the permitUnload roundtrip to timeout before we go ahead and close a tab. I'm reasonably certain we don't check to see if the parent process is already aware that the tab(s) belong to unresponsive processes.

If we know that to be true, we should probably just skip the permitUnload step and close the tab immediately.

Are you talking about the case where a content process is unresponsive enough that we've shown a "scripts are taking too long" notification bar? If so, I'm not sure if this is an exact duplicate, but this seems very similar to bug 1350060.

When reading this bug, I first wondered if you were also thinking about content processes that become unresponsive when we try to close a bunch of tabs at once. This is typically because closing tabs triggers potentially expensive GCs, ie. bug 1629064.

And there's also bug 1379174 about running permitUnload in multiple content processes at once, which I think might become more important with fission.

Marking as fxperf:p3 for now as that's the priority of most bugs related to tab closing performance, but I'm open to discussion about this.

Comment 2

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #1)

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #0)

We currently wait 1 second for the permitUnload roundtrip to timeout before we go ahead and close a tab. I'm reasonably certain we don't check to see if the parent process is already aware that the tab(s) belong to unresponsive processes.

If we know that to be true, we should probably just skip the permitUnload step and close the tab immediately.

Are you talking about the case where a content process is unresponsive enough that we've shown a "scripts are taking too long" notification bar?

Yes.

If so, I'm not sure if this is an exact duplicate, but this seems very similar to bug 1350060.

I think this is the timeout we have for the message that this bug is talking about skipping. That is, to do a permitUnload check, we need to talk to the child process, and there's a timeout after which we give up waiting for an answer. The point of 1350060 is to change the length of this timeout. The point of this bug is, if we've shown a slow script warning triggered by the process hang monitor, then we pretty much know the child is not going to respond anyway, even before we send the message, so we might as well not send the message and then wait for the length of the timeout.

When reading this bug, I first wondered if you were also thinking about content processes that become unresponsive when we try to close a bunch of tabs at once. This is typically because closing tabs triggers potentially expensive GCs, ie. bug 1629064.

And there's also bug 1379174 about running permitUnload in multiple content processes at once, which I think might become more important with fission.

Also interesting, but different. :-)

Marking as fxperf:p3 for now as that's the priority of most bugs related to tab closing performance, but I'm open to discussion about this.

So I think this one is a bit worse than ones for general tab closing performance, as the hypothesis is that users will try to close tabs or windows that show "slow script" warning messages, which will now wait for the aforementioned timeout until we give up on asking for beforeunload information (if we've recorded that the tab in question has a beforeunload handler). This may also be an interaction for which we'll reduce friction further with changes to the slow script UI (tbd). I also think making this change should be a simple additional if condition. So I think making this instantaneous is a quick usability win, and we should prio higher. In fact, let me see if I can put up a quick patch...

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1689462 - don't check for beforeunload on hung content processes, r?florian

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bf27fa825da
don't check for beforeunload on hung content processes, r=florian

Comment 5

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4bf27fa825da