Closed Bug 492410 Opened 11 years ago Closed 10 years ago

Disable slow script warning for chrome on branch

Categories

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

1.9.1 Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sdwilsh, Assigned: johnath)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

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.
Flags: blocking1.9.1?
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.
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.
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.
(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?
#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?
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).
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.
(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.
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).
(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.
(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.
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.
(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?
(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.
(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...
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?
(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
(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).
(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?
(In reply to comment #16)
> and this characteristic is inherent to the approach

I can't read :(
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.
Assignee: general → johnath
Attachment #378346 - Flags: review?(bent.mozilla)
Attachment #378346 - Flags: review?(bent.mozilla) → review+
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.
Assignee: johnath → nobody
Component: JavaScript Engine → DOM
Flags: blocking1.9.1? → blocking1.9.1+
QA Contact: general → general
Johnathan, I'm giving this to you since you wrote the patch. Let me know if you won't be able to land this...
Assignee: nobody → johnath
Whiteboard: [can land, branch only]
Version: Trunk → 1.9.1 Branch
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Summary: Slow script dialog for chrome scripts seems odd → Disable slow script warning for chrome on branch
Whiteboard: [can land, branch only]
Blocks: 490035
Faaborg said he ran into this when printing to PDF. He'll chime in with STR. For now, re-opening.
Status: RESOLVED → REOPENED
Keywords: fixed1.9.1
Resolution: FIXED → ---
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?
Group: mozilla-confidential
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?
Group: mozilla-confidential
Shaver: yeah, Faaborg confirmed it was from a webpage, so bug 492560; re-resolving and blocking on that one.
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.