Open Bug 1164286 Opened 9 years ago Updated 2 years ago

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

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

Tracking Status
e10s + ---
firefox41 --- affected

People

(Reporter: jujjyl, Unassigned)

References

(Blocks 3 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Attached file infiniteloop2.html
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.
tracking-e10s: --- → ?
Phillip, the suggestion here is that the stop and reload buttons should stop the script automagically. Does that make sense to you?
Flags: needinfo?(philipp)
Yeah, that makes total sense to me :)
Flags: needinfo?(philipp)
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.
Priority: -- → P2
Attached patch stop_script.patch (obsolete) — Splinter Review
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.
Assignee: nobody → blassey.bugs
Attachment #8735471 - Flags: review?(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.
Attachment #8735471 - Flags: review?(mconley) → review+
Attachment #8735471 - Attachment is obsolete: true
Attachment #8736131 - Flags: review?(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.
Attachment #8736131 - Flags: review?(mconley) → review+
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
Priority: P2 → P3
Assignee: lassey → nobody
Blocks: 1652455

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

Flags: needinfo?(mconley)
Summary: The page load stop and refresh buttons do nothing when the E10S slow script dialog is being shown. → The page load stop and refresh buttons do nothing when the E10S slow script notification is being shown.

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. :/

Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)

Looks like bug 1493225 reinvented bits of the wheel here.

Depends on: 1493225

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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugs)

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.

Flags: needinfo?(bugs)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: