901364 - Move operation callback machinery off of nsJSContext

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

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?

Comment 1

[Boris Zbarsky [: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.

Comment 2

[Bobby Holley (:bholley)]

(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.

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=6da71516f743

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist slow script prompt machinery into nsGlobalWindow. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement a replacement operation callback in XPConnect. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Switch to the XPConnect operation callback for DOM windows. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Make operation callbacks runtime-wide. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove a bunch of now-unused modal state tracking machinery. v1

Comment 9

[Bobby Holley (:bholley)]

We're going to want some heavy QA to play around with the new slow script dialog behavior once this lands.

Comment 10

[Bobby Holley (:bholley)]

Some stuff was [noscript] in that last try push that shouldn't have been:

https://tbpl.mozilla.org/?tree=Try&rev=2cf59e5aba28

Comment 11

[Bobby Holley (:bholley)]

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 12

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 15

[Bobby Holley (:bholley)]

Already had a green push in comment 11, but one more just to be sure:
https://tbpl.mozilla.org/?tree=Try&rev=de20bbcf49d2

Comment 16

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/mozilla-central/rev/6a89bd52a04d
remote:   https://hg.mozilla.org/mozilla-central/rev/5f749e3a4493
remote:   https://hg.mozilla.org/mozilla-central/rev/3f73532b694c
remote:   https://hg.mozilla.org/mozilla-central/rev/c698c9d89b91
remote:   https://hg.mozilla.org/mozilla-central/rev/72bc1aebb5d0

Comment 17

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old