1328264 - Timeouted and closed tab can still continue code execution

Status: NEW

(Firefox :: General, defect)

Description

[sworddragon2]

Attachment: testcase.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161213225349

Steps to reproduce:

1. In the attachments is a testcase which needs just to be opened and then closed immediately.
2. Wait 5 seconds until the tab gets closed.
3. Wait another 5 seconds.


Actual results:

The code from the closed tab still executes and causes the "A web page is slowing down your browser. What would you like to do?" dialog to appear which also prevents to use any website until the code gets stopped here.


Expected results:

:Gijs asked me to report this issue. I think the code execution should also stop if the tab gets closed from the timeout.

Comment 1

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

Mike, can you point me to where the per-tab slow script stuff was/is implemented? Do we know if the tab is closed there (or at some point in the parent) and can we auto-decide for the user based on that knowledge?

Comment 2

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

(In reply to :Gijs from comment #1)
> Mike, can you point me to where the per-tab slow script stuff was/is
> implemented? Do we know if the tab is closed there (or at some point in the
> parent) and can we auto-decide for the user based on that knowledge?

Hey - sorry for the delay on this. I believe this is the ProcessHangMonitor stuff from billm that I reviewed a while back... lemme see if I can find it.

http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/browser/modules/ProcessHangMonitor.jsm

I wonder if this needs to be updated to handle TabClose?: http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/browser/modules/ProcessHangMonitor.jsm#354-356

Comment 3

[ovidiu boca[:Ovidiu]]

Gijs do you have any component suggestion for this bug?

Comment 4

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

(In reply to ovidiu boca[:Ovidiu] from comment #3)
> Gijs do you have any component suggestion for this bug?

bug 1164931 was in Fx::General, so I guess this one should be, too.

(In reply to Mike Conley (:mconley) from comment #2)
> I wonder if this needs to be updated to handle TabClose?:
> http://searchfox.org/mozilla-central/rev/
> 0f254a30d684796bcc8b6e2a102a0095d25842bb/browser/modules/ProcessHangMonitor.
> jsm#354-356

That looks right to me, though I think we should immediately call terminateScript or terminatePlugin for those windows in that case. Bill, does that sound right?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

One problem is that the ProcessHangMonitor code doesn't run until the 10 second script timeout. That's the reason for the last "wait 5 seconds" step I guess. If we made the two timeouts the same, then this would be less of a problem, although it would still be nice closing the tab automatically killed slow scripts in that tab.

Regarding changing the timeout: this is tricky because people generally hate seeing the slow script infobar. It might be better to have a 5 second internal script hang timeout but avoid reporting the hang until the full 10 seconds *except* when the tab is closed. That could be done by modifying the timeout pref and making changes to ProcessHangMonitor.jsm.

Regarding terminating scripts connected to closed tabs: this shouldn't be too hard. If we get a hang report in [1], we can look at report.scriptBrowser and see if it has been closed (or maybe watch to see if it's closed). In that case we can act as if the user hit "Stop script".

[1] http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/browser/modules/ProcessHangMonitor.jsm#363

Comment 6

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

I can no longer reproduce this on nightly with the steps from comment #0 - seems like the tab closing now takes effect more immediately? Bill, did this get fixed elsewhere, and/or do we still need the suggested fix in comment #5 ?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This still happens. It's masked by the fact that we have 4 content processes now.

Comment 8

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

Mike, any chance this was fixed by bug 1391594, or that you know what pieces remain here?

Comment 9

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

(In reply to :Gijs (no reviews; PTO recovery mode) from comment #8)
> Mike, any chance this was fixed by bug 1391594, or that you know what pieces
> remain here?

Hm. Bug 1391594 dealt mostly with window closure and shutdown. The STR talk about closing the tab after the script starts running, and I don't think my patches would have changed our behaviour in that case.

Comment 10

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

I can still reproduce this issue, fwiw - I just have to ensure that the process that I'm running the attachment in is also being used by another tab (otherwise we force-kill the process once the tab closes).