Closed Bug 1270729 Opened 3 years ago Closed 3 years ago

When stopping slow scripts, totally halt script execution.

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(1 file)

There is an XPConnect flag for killing a tab's ability to run scripts entirely. We should investigate using this.
Component: XPCOM → General
Blocks: 1269917
No longer blocks: 1269917
Depends on: 1269917
Assignee: nobody → winter2718
No longer depends on: 1269917
Blocks: 1269917
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.
Attached patch slowscript.diffSplinter Review
Not sure who should review this yet. The change is extremely small.
Component: General → XPConnect
(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.
Whiteboard: btpp-active
Flags: needinfo?(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.
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! :)
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
(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?"
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 on attachment 8752704 [details] [diff] [review]
slowscript.diff

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

This was suspiciously easy.
Attachment #8752704 - Flags: review?(bobbyholley)
(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 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.
Attachment #8752704 - Flags: review?(bobbyholley) → review+
Sounds good to me. Thanks for driving this, mrrrgn!
Flags: needinfo?(mconley)
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.
Blocks: 1251545
https://hg.mozilla.org/mozilla-central/rev/e40a0a79995f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1275416
You need to log in before you can comment on or make changes to this bug.