The page load stop and refresh buttons do nothing when the E10S slow script notification is being shown.
Categories
(Firefox :: General, defect, P3)
Tracking
()
People
(Reporter: jujjyl, Unassigned)
References
(Blocks 3 open bugs)
Details
Attachments
(3 files, 1 obsolete file)
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.
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
Comment 3•9 years ago
|
||
Phillip, the suggestion here is that the stop and reload buttons should stop the script automagically. Does that make sense to you?
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.
Updated•8 years ago
|
Comment 6•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Comment 9•8 years ago
|
||
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•6 years ago
|
||
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
Updated•5 years ago
|
Comment 11•4 years ago
|
||
Mike, do you know why the patch here never landed?
Comment 12•4 years ago
|
||
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. :/
Updated•4 years ago
|
Comment 13•4 years ago
|
||
Looks like bug 1493225 reinvented bits of the wheel here.
Comment 14•4 years ago
|
||
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•4 years ago
|
||
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.
Updated•2 years ago
|
Description
•