1270729 - When stopping slow scripts, totally halt script execution.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Morgan Phillips [:mrrrgn]]

There is an XPConnect flag for killing a tab's ability to run scripts entirely. We should investigate using this.

Comment 1

[Morgan Phillips [:mrrrgn]]

Note: this concept is subtly different from the current "stop script" mechanism. Rather, this would disable all scripts on a tab. There are a couple of ways to approach this in my view:

1.) Replace "stop script's" current behavior with this disable script behavior. This doesn't seem ideal since many users may stop a script and continue interacting with a page (we could get data to back this up).

2.) Move from "stop script" to "disable JS" behavior after the first stop script dialog is shown for a page. This could confuse users since the "stop script" button's behavior would be changing based on how many times it had appeared.

3.) Offer a "disable JavaScript" option alongside "stop script." Most users will not understand this distinction nor should they be expected to.

Comment 2

[Morgan Phillips [:mrrrgn]]

Attachment: slowscript.diff

Not sure who should review this yet. The change is extremely small.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Morgan Phillips [:mrrrgn] from comment #2)
> Not sure who should review this yet. The change is extremely small.

I don't either. It's not the size of the change. It's the fact that this is really a UI/UX design decision.

Comment 4

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

This kind of feels like pause / play. A quick look inside XPCJSRuntime.cpp shows the ability to unblock a script as well.

I think that might be a straight-forward way of communicating to the user of what is happening here.

So perhaps that's how we should frame it - instead of stopping a script, we communicate the idea of pausing all page scripts, maybe with the added ability to play them again if they paused them by accident.

Going along those lines, that means that we can have two-states for JS execution in each tab, where the playing state is the one that's default and the common-case. The paused state is the one that we'll need to communicate to the user - perhaps with a notificationbar or something.

I just talked to shorlander, and he suggested I sketch something up and he can give feedback. Gonna keep the needinfo on myself to do that.

Comment 5

[Jason Orendorff [:jorendorff]]

This sounds pretty great.

The devtools Debugger tab also has a pause/play button. The meaning is a bit different. If you hit the "Stop It" button in the slow script dialog, the running JS is killed, as if it hit a mysterious uncatchable exception and died. But if you pause running JS code in the debugger, it really is just paused. When you hit play, it continues whatever it was doing.

None of which really affects the discussion, I don't think. I'm bringing this up for no reason. These are some things that I know. Feel free to disregard! :)

Comment 6

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

Still getting my thoughts together, but here is where I'm headed if people are interested in following along: https://www.evernote.com/l/AbIWcecKvd5EZprMZxy8Sl39ixBoq1KkR7Y

Comment 7

[Morgan Phillips [:mrrrgn]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #6)
> Still getting my thoughts together, but here is where I'm headed if people
> are interested in following along:
> https://www.evernote.com/l/AbIWcecKvd5EZprMZxy8Sl39ixBoq1KkR7Y

The idea of pausing a tab makes a lot of sense here. In fact, that language is probably less confusing for non-technical users who probably wonder: "what is a script and why am I stopping it?"

Comment 8

[Morgan Phillips [:mrrrgn]]

After talking about this some more I believe that it's worthwhile to just land this change. Clicking 'stop script' implies that scripts on a page will actually stop (without potentially being retriggered by retry/event code).

Comment 9

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8752704 [details] [diff] [review]
slowscript.diff

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

This was suspiciously easy.

Comment 10

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

(In reply to Morgan Phillips [:mrrrgn] from comment #8)
> After talking about this some more I believe that it's worthwhile to just
> land this change. Clicking 'stop script' implies that scripts on a page will
> actually stop (without potentially being retriggered by retry/event code).

I agree that this is probably what's intended by the user. I do think, however, it'd be useful to get some data on what users do and what they expect when they see this dialog / notification.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8752704 [details] [diff] [review]
slowscript.diff

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1444,5 @@
>  
>      // Show the prompt to the user, and kill if requested.
>      nsGlobalWindow::SlowScriptResponse response = win->ShowSlowScriptDialog();
> +    if (response == nsGlobalWindow::KillSlowScript) {
> +        if (Preferences::GetBool("dom.global_stop_script"))

I think we should pass aDefault=true here.

Comment 12

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

Sounds good to me. Thanks for driving this, mrrrgn!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e40a0a79995f

Comment 14

[Morgan Phillips [:mrrrgn]]

For analysis it would probably be interesting to see how quickly/often users reload a page after stopping a script. Even then it's not a super strong signal. That being the case analysis here will take a long time -- it's also worth noting that, according to billm, stop script dialogues are happening less frequently than crashes ~ every 180 hours.

Because it's reasonable to think that 'stopping a script' will halt all execution on a page and this patch stands a chance of improving the e10s experience (no more multiple dialogues) my current plan is to push forward with landing.

I believe that further UI changes and/or analysis would be wonderful but should be split off into their own bugs.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e40a0a79995f