1164286 - The page load stop and refresh buttons do nothing when the E10S slow script notification is being shown.

Status: NEW

(Firefox :: General, defect, P3)

Description

[Jukka Jylänki]

Attachment: infiniteloop.html

STR:

1. Open infiniteloop.html in browser.
2. Wait until the slow script notification appears: "A web page is causing Nightly to run slowly. What would you like to do?"
3. Press the Cross icon at the top of the navigation bar to the right of the page URL to stop loading the page. (instead of responding to the slow script bar)

Observed: The cross icon renders a "clicked"/"activated" visual state when clicked with the mouse, but nothing happens.

Expected: I believe pressing the cross icon should have the same effect as choosing "Stop script"? Or otherwise the cross icon should not appear, or it should show a grayed out image, instead of rendering a pressed state image upon clicking.

Comment 1

[Jukka Jylänki]

Attachment: infiniteloop2.html

Comment 2

[Jukka Jylänki]

STR 2:

1. Open infiniteloop2.html in browser.
2. Wait until the slow script notification appears: "A web page is causing Nightly to run slowly. What would you like to do?"
3. Press the page Refresh/Reload icon at the top of the navigation bar to the right of the page URL to stop loading the page. (instead of responding to the slow script bar)

Observed: The page Refresh/Reload icon renders a "clicked"/"activated" visual state when clicked with the mouse, but nothing happens. The page does not reload.

Expected: Pressing the page Refresh/Reload icon should stop executing any scripts on the page and immediately reload the page.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Phillip, the suggestion here is that the stop and reload buttons should stop the script automagically. Does that make sense to you?

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yeah, that makes total sense to me :)

Comment 5

[Karla Merza [Away. Please needinfo? Brindusa Tot[:brindusat] ]]

User Agent  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:46.0) Gecko/20100101 Firefox/46.0
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID  20160208004010

User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
User Agent  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID  20160208030244

Tested on Windows 7 x64 and Mac OS 10.10 with the Aurora 46.0a2 build and Nightly 47.a01 build. 
I can reproduce the problem.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: stop_script.patch

This handles the case where we have a hang report. I think we should file a follow up to figure out what we can do when the user clicks stop or reload while a script is hung but we haven't generated a report yet.

Comment 7

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

Comment on attachment 8735471 [details] [diff] [review]
stop_script.patch

Review of attachment 8735471 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good idea, though we might want to do the same thing with loading new pages (see below).

I think this is important enough to take as is, but I'd love to see some tests for this as well. If you don't want to block on writing new tests for this, can you file a new bug to get some tests written? The tests could go in https://dxr.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873/browser/modules/test/browser_ProcessHangNotifications.js#174.

::: toolkit/components/remotebrowserutils/RemoteWebNavigation.js
@@ +90,5 @@
>      });
>    },
>    reload: function(aReloadFlags) {
> +    // If there is a hang report, stop the script
> +    ProcessHangMonitor.stopIt(this._browser.ownerDocument.defaultView)

We might want to do the same thing for loadURIWithOptions, so that if the user attempts to browse away from an unresponsive page, we stop the script.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: stop_script.patch

Comment 9

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

Comment on attachment 8736131 [details] [diff] [review]
stop_script.patch

Review of attachment 8736131 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the test!

::: browser/modules/test/browser_ProcessHangNotifications.js
@@ +187,5 @@
>    yield promise;
>  });
> +
> +/**
> + * Test if hang reports receive a terminate script callback when the user reloads

"reloads" -> "stops"

@@ +191,5 @@
> + * Test if hang reports receive a terminate script callback when the user reloads
> + * the page.
> + */
> +
> +if (gBrowser.selectedBrowser.isRemoteBrowser) {

This entire test file is only ever run if e10s is enabled[1], so I think we can skip this bit.

[1]: https://hg.mozilla.org/mozilla-central/file/d1d47ba19ce9/browser/modules/test/browser.ini#l7

@@ +196,5 @@
> +  add_task(function* terminateOnReloadScriptTest() {
> +    let promise = promiseNotificationShown(window, "process-hang");
> +    Services.obs.notifyObservers(gTestHangReport, "process-hang-report", null);
> +    let notification = yield promise;
> +    

Trailing ws

@@ +199,5 @@
> +    let notification = yield promise;
> +    
> +    gTestHangReport.hangType = gTestHangReport.SLOW_SCRIPT;
> +    promise = promiseReportCallMade(gTestHangReport.TEST_CALLBACK_TERMSCRIPT);
> +    window.gBrowser.stop();

This is good. We should probably add more tests for the navigation, load URL, and reload cases. Let's just land this now though and do that in a follow-up.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 11

[:Gijs (he/him)]

Mike, do you know why the patch here never landed?

Comment 12

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

Wow, yes - looks like the patch went through review, and just never got landed. No checkin-needed was set, nobody tried to push it to integration/fx-team (which I guess was the technique at the time). I think this was an oversight. :/

Comment 13

[:Gijs (he/him)]

Looks like bug 1493225 reinvented bits of the wheel here.

Comment 14

[:Gijs (he/him)]

Olli, I'm trying to resurrect the patch here, but I noticed that back/forward/go-to-history-index and navigation are already doing something like this, via maybeCancelContentJSExecution - but we do not do this for stop and reload (and in fact, the navigation code has an explicit escape-clause that avoids cancelling JS when the new URL is the same (ignoring ref). You reviewed the patches in bug 1493225.

Is the "right" fix here to opt the refresh and stop behaviours into the same mechanism, or is that likely to cause other issues and should I use the process hang monitor the way the attached patch does? Although I've seen that the maybeCancelContentJSExecution stuff works via docshell, I can't quite figure out if/how it'd stop scripts in the way the slow script notification bar does, and/or if it (for instance) wouldn't stop extension script running against the same webpage.

TL;DR: how does nsIRemoteTab's canceling of content JS compare to the process hang monitor, and which one do we want here?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

In bug 1493225 we wanted to limit the scope a bit, in order to make it a bit less risky.
But I think that mechanism should work well with reloads. I guess stop shouldn't be too bad either, since the page is most probably broken anyhow
if stop is pressed.
The underlying mechanism maybeCancelContentJSExecution uses should be similar to slow script notification - get a JS interruption callback call and inform that script execution should be stopped.

Cancelling content JS uses PProcessHangMonitor so that we have a communication channel outside PContent.