492410 - Disable slow script warning for chrome on branch

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Shawn Wilsher :sdwilsh]

We've seen a number of bugs popping up about the slow script dialog coming up for chrome scripts that never used to pop up (and some of these cases appear to be faster than before the recent changes to this).

We'd really like to be able to disable the the slow script dialog for chrome scripts since it'll be incredibly confusing to users about some chrome script being unresponsive.

One specific case mentioned by beltzner was if he takes too long to select his printer, he'll get asked to stop a script.  This is a really bad user experience.

See also bug 490035 and bug 492197.

Comment 1

[Johnathan Nightingale [:johnath]]

Personally, I like these dialogs a lot because they help us feel the pain of slow-running chrome JS - but I would expect them to be treated like assert()s - enabled for debug builds, disabled for releases.

I don't think our users are ahead by seeing a dialog pop up talking about a misbehaving script on a page when they're interacting with our UI.  But I DO like having this alarm bell during development.

Comment 2

[Andreas Gal :gal]

What changed is that the dialog now comes up strictly based on time. Previously it was some hand-calibrated "number of instructions" metric. If we don't want slow script dialogs in certain places, we have to disable them straight at the source in the dialog callback. Probably not hard though.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

We already are able to disable it:
dom.max_chrome_script_run_time = -1

That we need to now when we didn't before is what's worrying, though. It might just be due to the fact that this timeout is implemented differently, but the people who implemented it are best suited to comment about that, I think.

I think the printing case beltzner and I have seen is its own bug that's potentially related to Cocoa widget dialog hacks, and I'll file it separately. I don't think we should conflate it with bugs like bug 490035 or bug 492197 that are caused by script actually taking a long time to run.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> What changed is that the dialog now comes up strictly based on time. Previously
> it was some hand-calibrated "number of instructions" metric.

Are we now including time spent in native code called by script, when we used to only measure time spent running the script itself?

Comment 5

[Andreas Gal :gal]

#4: yes, strictly total script execution time, including time spent in natives. The engine goes to great length to make sure that native code running is subject to the timeout (this is very important for content). If we want slow script dialogs for chrome, maybe we want a different timeout?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Alternatively, would be cool to have a way to mark a call as "known-to-be-slow", so that the dialog won't come up for that in optimized builds.

If we go for a "different timeout", well it will be hard to find what is an acceptable value, for example importing a normal bookmarks profile from another browser does not create any issue, but importing a large profile will fire the dialog, i can't imagine what is a good measure of "large" (we have users with 70'000 bookmarks, that's crazy but true).

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Any time we hit a slow script dialog it means that we've been away from the main event loop for far too long and that the UI has been effectively frozen. That's bad. IMHO we should not try to whitelist anything or extend the timeout but rather find the slow scripts and fix them.

Comment 8

[Johnathan Nightingale [:johnath]]

(In reply to comment #7)
> Any time we hit a slow script dialog it means that we've been away from the
> main event loop for far too long and that the UI has been effectively frozen.
> That's bad. IMHO we should not try to whitelist anything or extend the timeout
> but rather find the slow scripts and fix them.

No debate, but as I say, it doesn't help our users to have a "poor man's profiling" dialog popping up in the middle of their browsing session. That's why I think it makes sense to wrap it in a debug ifdef.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I think the purposed solution looks similar to the "slow query warnings" in mozStorage, we can disable warnings with a comment so we don't make developers angry, and we file a bug on the slow query, fixing it asap. In some situation changes needed to fix one of those warnings require big re-architecture or idl changes (not possible when we are near a release).

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Any time we hit a slow script dialog it means that we've been away from the
> main event loop for far too long and that the UI has been effectively frozen.
> That's bad. IMHO we should not try to whitelist anything or extend the timeout
> but rather find the slow scripts and fix them.
While this is true, this behavior changed late in the 1.9.1 schedule limiting the kinds of changes we can make to fix these cases.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #8)
> That's why I think it makes sense to wrap it in a debug ifdef.

Well, we can just use preprocess magic to make the default max_chrome_script_run_time pref -1 for debug builds, if it comes to that.

Comment 12

[Ria Klaassen (not reading all bugmail)]

Maybe a progress indicator or a message "still working" because it's not always clear if firefox is still busy with the task. You don't always see the hourglass cursor.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #7)
> Any time we hit a slow script dialog it means that we've been away from the
> main event loop for far too long and that the UI has been effectively frozen.

That isn't the case here, right, since the modal dialog box spins a sub event loop?

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #13)
> That isn't the case here, right, since the modal dialog box spins a sub event
> loop?

It unfreezes with the display of the dialog, yes, but it was frozen for the previous ten seconds.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> #4: yes, strictly total script execution time, including time spent in natives.

The more important part of my question was whether that was a behavior change from the previous slow-script dialog implementation (intentional or not). I'm assuming it is?

I think Shawn is right that we're pretty late in the cycle to be addressing some of these slowdowns at their source, and Johnathan is right that the user experience in these cases is likely not helped by the dialog appearing. I'm not sure where that leaves, us, though. I'm not familiar with the specifics here, apart from knowing that it took Igor and others some effort to come up with the current system, but if there was a change in behavior with regard to native code vs. script, is reverting that behavior change feasible? It might also help address bug 492560...

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Reverting the change isn't really feasible -- probably riskier than addressing these at their sources, IMO, since it took quite a while to get right, and this characteristic is inherent to the approach (not a knob we can easily turn, or even add and then turn!).

We can just disable slow-script for chrome, though, if that's what we want?

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> We can just disable slow-script for chrome, though, if that's what we want?
Personally, I think that's the best solution for 1.9.1

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #14)
> It unfreezes with the display of the dialog, yes, but it was frozen for the
> previous ten seconds.

Sorry, I'm being dense... The main UI was frozen for the previous ten seconds because of a modal dialog that the user was presumably interacting with. There was no point in time where everything was literally hung (which is why the dialog exists in the first place).

I guess I'm asking why we can't just reset the operation time when we return from a modal dialog (only for chrome, of course).

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> Reverting the change isn't really feasible -- probably riskier than addressing
> these at their sources, IMO

To be clear, "the change" I was asking about is not the entire new slow script dialog implementation (i.e. bug 453157), but rather the change in behavior it introduced with regard to how it treats native code specifically. Am I right to infer from your reply that that behavior change is inherent to the new system?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> and this characteristic is inherent to the approach

I can't read :(

Comment 21

[Johnathan Nightingale [:johnath]]

Attachment: Flip the pref by default

Just flips the pref off for chrome. Had a conversation with shaver about whether it made sense to leave it enabled for debug builds, but his argument was that anyone actually interested in spotting these things can just change the pref off-default.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 378346 [details] [diff] [review]
Flip the pref by default

To be clear, this is proposed for branch only?

I think it's fine, though I tend to like 0 better than -1 but the code handles both identically.

Comment 23

[Johnny Stenback (:jst)]

Johnathan, I'm giving this to you since you wrote the patch. Let me know if you won't be able to land this...

Comment 24

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b24bf4b15d55

Given that we decided to leave this on trunk, I'm going to morph the summary and mark this fixed.

bent - landed with 0 instead of -1.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Faaborg said he ran into this when printing to PDF. He'll chime in with STR. For now, re-opening.

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think that's a different bug, which we made non-blocking because of this (apparently insufficient) mitigation -- chrome definitely has the slow script warning disabled, so I think this is RESOLVED FIXED and bug 492560 should perhaps be reopened?

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Alex: any chance the print to PDF operation was after clicking a button in a webpage that said "Click to print this page" or similar?

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Shaver: yeah, Faaborg confirmed it was from a webpage, so bug 492560; re-resolving and blocking on that one.