1456241 - dropbox shared image link page is very slow to close

Status: RESOLVED INCOMPLETE

(Firefox :: Tabbed Browser, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

+++ This bug was initially created as a clone of Bug #1451985 +++

Cloning this off to track the performance around closing the page.

STR:

1. Open a shared dropbox link like this:

https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0

2. Close the tab.
3. Observe that it takes many seconds for the tab to close.

Comment 1

[Andrew Overholt [:overholt]]

Looking at a profile here, it seems maybe it's a long beforeunload handler being run.

Comment 2

[Andrew Overholt [:overholt]]

I know we made some changes in the past to make tab closing visually fast but run the beforeunload handlers in the background. Mike, is that not working as intended here?

Comment 3

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

(In reply to Andrew Overholt [:overholt] from comment #2)
> I know we made some changes in the past to make tab closing visually fast
> but run the beforeunload handlers in the background. Mike, is that not
> working as intended here?

What we do is skip sending a message to the content to run the beforeunload event handlers if we know no beforeunload event handler was set.

If, however, content _has_ set a beforeunload event handler, we send it a message, and we wait for a maximum of 1s for it to complete running that function before bailing out: https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/toolkit/content/widgets/remote-browser.xml#321

I'm not certain why it'd take many seconds - it's supposed to take a maximum of 1s.

tbh, we might want to consider ratcheting that timeout down even further.

bkelly, are you still seeing it take many seconds?

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

I see it taking perhaps 4 seconds on this over-the-forest-under-the-lake network connection.

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

https://perfht.ml/2nHc5ME

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

There is sync XHR running inside beforeunload listener.

Comment 7

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

Oh yikes - it looks like we've got code in here that intentionally disables the timeout if the content process has responded:

https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/toolkit/content/widgets/remote-browser.xml#335

We should probably re-evaluate that.

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Sync XHR is probably one of the most common time consuming operations happening during beforeunload.
We should probably hide the tab rather soon if we detect sync XHR, not kill the tab, but hide it.

Comment 9

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

(In reply to Olli Pettay [:smaug] from comment #8)
> Sync XHR is probably one of the most common time consuming operations
> happening during beforeunload.
> We should probably hide the tab rather soon if we detect sync XHR, not kill
> the tab, but hide it.

I think we've considered it in the past. As I recall, the concern was "what do we do if the sync XHR finishes, and then beforeunload returns a signal saying to not permit the unload"? Presumably, in that case, the tab has to re-appear, and I think we never found a great way of doing that which wasn't jarring for the user.

But it's definitely worth revisiting.

Comment 10

[:Gijs (out for now; he/him)]

Tentatively setting fxperf:p2 . I think this would be good to fix but also quite hairy (having the tab re-appear if a prompt is required, and more generally how to detect we need to reshow the tab - e.g. what if a permissions request fires?).


Other notes: I also think this is kind of a dupe of bug 1103323, and I can't reproduce with the link from comment #0 (404).

Comment 11

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

Another thing we might reconsider is skipping the timeout if content has responded. Maybe we should just have a 1sec (or 500ms?) cap, no matter what. This, of course, risks data loss on pages that are trying to save things with onbeforeunload...

Comment 12

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Would anyone have time to test other browsers? Some simple test case using sync XHR during beforeunload and a slow response from network.

Comment 13

[Alex Thayer [:alexical] (she/her)]

I am closing this bug out as it is unlikely to ever actually get traction with the current broken STR. If the XHR issue is still relevant, which I'm guessing it is, I think we could probably create a simpler and more stable STR for the issue, which may need to happen in a fresh bug, but it seems like this is still rather low priority.