1379174 - When closing multiple tabs, the PermitUnload message should be sent to each tab in parallel

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Florian Quèze [:florian]]

Currently the message is sent to one tab, then we spin the event loop until we have a reply: http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/toolkit/content/widgets/remote-browser.xml#344

This is very slow when closing lots of tabs, especially when the reason for closing many tabs is that too much memory is being used, because in that case the content processes will be most likely busy gc'ing. We'll have to wait until it is done GC'ing before getting a reply, but it would be nice if it could reply at once for all the tabs that it owns.

Here is a profile where closing ~160 tabs spent 5.5+s in permitUnload: https://perfht.ml/2toSPHw

Comment 1

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

Here's how we might approach this:

1) For the set of closed tabs, find which ones have beforeunload set (via the nsITabParent hint), and close those immediately.
2) For the remaining tabs, group them by which process they're in, and send a single message to each process with some kind of unique identifier for each tab that's closing.
3) Spin the event loop until either:
  a) We've heard back from each content process, and can choose whether or not to abort the unload for one or more tabs
  b) Some very small timeout has been exceeded

Inside the content process, we'll need something to receive that single message, find the associated top-level windows, and run the permitUnload function on each, serially. I think it might be better to message the parent as soon as we find a page that wants to block unloading, instead of waiting until all pages have run (since we can't be sure how long running them all will take).

Another thing we could do is continue to close tabs while the user is trying to decide whether or not to allow the tab / window to close.

Comment 2

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

(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #1)
> Another thing we could do is continue to close tabs while the user is trying
> to decide whether or not to allow the tab / window to close.

To be clear, I mean, if we have tabs A, B, C and D, all running in the same content process, and B wants to block unloading, we might want to do:

1) Run A's permitUnload, find that it doesn't want to block, move onto the next...
2) Run B's permitUnload, find that it _DOES_ want to block, message parent immediately, and then...
3) Run C and D's permitUnload without waiting for any kind of response from the parent. Keep B open in the meantime.

Comment 3

[Florian Quèze [:florian]]

Attachment: Bug 1379174 - when closing multiple tabs, handle PermitUnload in parallel, r=Gijs.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7478f194bc6
when closing multiple tabs, handle PermitUnload in parallel, r=Gijs.

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/c7478f194bc6

Comment 6

[Florian Quèze [:florian]]

Here's a profile of closing about 600 tabs on a recent build: https://share.firefox.dev/2QPHEGb

We are no longer blocked on content processes, but this profile explains why we saw no improvement on TelemetryStopwatch FX_TAB_CLOSE_PERMIT_UNLOAD_TIME_MS: we stop recording the time after the parent has processed the "PContent::Reply_DispatchBeforeUnloadToSubtree" IPC... and that's blocked on the parent process main thread being busy.