Closed Bug 901364 Opened 6 years ago Closed 6 years ago

Move operation callback machinery off of nsJSContext

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files)

Right now, the watchdog invokes a callback on nsJSContext, which checks for the associated script context on the current cx, an bails if such a script context is not found. If it is found, it uses per-scriptcontext timestamps to figure out whether script has been running too long.

This is suboptimal for a number of reasons:
* It's a dependency on JSContext-per-DOM-window, which needs to go away
* If a nasty content script can trigger the usage of a different JSContext at the moment the watchdog fires (say, by creating an iframe and synchronously dispatching some event to it), the script can bypass the watchdog entirely.
* Our statistics and heuristics are generally less-meaningful.

Some simplification of this machinery is probably overdue here. One options is simply to track slow-scriptness by tracking the most recent trip through the event loop (via a stack, due to nesting), and determine chrome-vs-content just by examining the subject principal when the callback is fired. This isn't perfect (it's possibly to fool the subject principal check, and to spam the event loop), but it might be good enough, and very simple. Thoughts? Other suggestions?
Flags: needinfo?(bzbarsky)
This sounds similar to my proposal to change operation callbacks to consider wall-clock time since last processed task or outermost script entry point, yes.

Note that we'll need some way to reach an nsIPrompt that's relevant to the running script or something, but doing that via its global instead of the jscontext ought to be ok.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #1)
> This sounds similar to my proposal to change operation callbacks to consider
> wall-clock time since last processed task or outermost script entry point,
> yes.

Well, I think you suggested that we don't want to use script entry points (due to how event dispatch works), and that it was simplest just to do it when we cycle through the event loop.
Assignee: nobody → bobbyholley+bmo
Blocks: 903212
We're going to want some heavy QA to play around with the new slow script dialog behavior once this lands.
Keywords: verifyme
Some stuff was [noscript] in that last try push that shouldn't have been:

https://tbpl.mozilla.org/?tree=Try&rev=2cf59e5aba28
All green modulo one xpcshell orange that I think is related to the underlying changeset. Rebased on top of njn's JSBool removal:

https://tbpl.mozilla.org/?tree=Try&rev=31502b3f1d2e
Comment on attachment 787913 [details] [diff] [review]
Part 1 - Hoist slow script prompt machinery into nsGlobalWindow. v1

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

A couple of small comments, r=me with them addressed.

::: dom/base/nsGlobalWindow.cpp
@@ +9373,5 @@
>    return NS_OK;
>  }
>  
> +nsGlobalWindow::SlowScriptResponse
> +nsGlobalWindow::ShowSlowScriptDialog()

Please assert IsOuterWindow() (I think, am I reading the code right) here so it's clear what window we expect this to be called on.

@@ +9388,5 @@
> +  }
> +
> +  // Get the nsIPrompt interface from the docshell
> +  NS_ENSURE_TRUE(GetDocShell(), KillSlowScript);
> +  nsCOMPtr<nsIPrompt> prompt = do_GetInterface(GetDocShell());

Might as well store the result of GetDocShell() in a local variable, no?

@@ +9394,5 @@
> +
> +  // Check if we should offer the option to debug
> +  JS::RootedScript script(cx);
> +  unsigned lineno;
> +  JSBool hasFrame = ::JS_DescribeScriptedCaller(cx, script.address(), &lineno);

While you're here, mind nuking the :: before the JS_ calls?
Attachment #787913 - Flags: review?(mrbkap) → review+
Attachment #787914 - Flags: review?(mrbkap) → review+
Attachment #787915 - Flags: review?(mrbkap) → review+
Comment on attachment 787916 [details] [diff] [review]
Part 4 - Make operation callbacks runtime-wide. v1

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

::: dom/base/nsGlobalWindow.cpp
@@ +9517,5 @@
>    rv = prompt->ConfirmEx(title, msg, buttonFlags, waitButton, stopButton,
>                           debugButton, neverShowDlg, &neverShowDlgChk,
>                           &buttonPressed);
>  
> +  JS_SetOperationCallback(JS_GetRuntime(cx), old);

I'd pull the runtime out into a local variable.

::: js/xpconnect/shell/xpcshell.cpp
@@ +1466,5 @@
>  static bool
>  ContextCallback(JSContext *cx, unsigned contextOp)
>  {
>      if (contextOp == JSCONTEXT_NEW) {
>          JS_SetErrorReporter(cx, XPCShellErrorReporter);

Nit: braces should go away since the consequent is only one line now.
Attachment #787916 - Flags: review?(mrbkap) → review+
Attachment #787917 - Flags: review?(mrbkap) → review+
I asked bholley on IRC to make sure that things like calling window.print() and window.alert() from JS don't trigger the slow script dialog when we close them. I think they should be OK because they run the event loop and therefore we should be consistently resetting our wall clock start time, but I'm not 100% sure.
Already had a green push in comment 11, but one more just to be sure:
https://tbpl.mozilla.org/?tree=Try&rev=de20bbcf49d2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.