Bug 17133 – Should support pausing JavaScript execution without hanging the process

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Adam Roben (:aroben)]

Currently the only way to pause JavaScript execution is to block the entire process (which is the approach taken by Drosera). It would be much nicer if we could pause JavaScript execution without blocking the process. This would, for example, allow us to build an in-process JavaScript debugger, a la Firebug.

[Adam Roben (:aroben)]

See bug 8285 for a previous attempt at reworking the JS interpreter in a way that would have allowed pausing execution.

[Adam Roben (:aroben)]

<rdar://problem/5719551>

[Adam Roben (:aroben)]

It seems that Firebug, while paused at a breakpoint, basically stops any new JavaScript execution from being initiated. In particular:

1. JavaScript event handlers never run
2. Code scheduled to be executed via setTimeout/setInterval executes immediately when JS is resumed if the timer went off while paused

[Adam Roben (:aroben)]

I think we can follow Firebug/Gecko's model here in WebKit.

(In reply to comment #3)
> 2. Code scheduled to be executed via setTimeout/setInterval executes
> immediately when JS is resumed if the timer went off while paused

This doesn't seem like a particularly good behavior. I think it would be better to pause the timers themselves while JS is paused, and resume the timers once you resume.

[Adam Roben (:aroben)]

Another thing we'll have to pause are callbacks from the SQL storage API.

[Adam Roben (:aroben)]

We'll also want to block plugins from obtaining access to the DOM, and we'll probably want to pause all plugin requests, since they can request javascript: URIs.

[Adam Roben (:aroben)]

To actually pause at a breakpoint, I think we'll just need to spin up a nested event loop.

[Adam Roben (:aroben)]

Created attachment 19588 [details]
[WIP] Make it possible to pause JS timers and block JS event handlers per-Frame

[Adam Roben (:aroben)]

Created attachment 19589 [details]
[WIP] Make it possible to pause/resume the DatabaseThread

[Adam Roben (:aroben)]

Created attachment 19590 [details]
[WIP] Make it possible to pause/block JS from plugins (Windows only)

[Anders Carlsson]

The JS plugin approach seems good but the patch is insufficient. Since a plug-in can hold on to any NPObject, any NPRuntime call that can result in JS being executed needcs to be handled in the same way as NPN_GetValue.

[Adam Roben (:aroben)]

Anders thinks that maybe the current patch, while theoretically insufficient, might be good enough, practically speaking:

<andersca> aroben: I think that all the plug-ins I've encountered always get the window NPObject before calling JS
<andersca> aroben: so for now you might be able to get away with just preventing getvalue

[Adam Roben (:aroben)]

(In reply to comment #11)
> The JS plugin approach seems good but the patch is insufficient. Since a
> plug-in can hold on to any NPObject, any NPRuntime call that can result in JS
> being executed needcs to be handled in the same way as NPN_GetValue.

If we decide to address this issue we will have to patch the methods in WebCore/bridge/NP_jsobject.cpp.

[Timothy Hatcher]

On Mac we will need to prevent WebScriptObject from making any calls. The evaluate methods and setValue:forKey:. I can look at doing this.

[Adam Roben (:aroben)]

(In reply to comment #14)
> On Mac we will need to prevent WebScriptObject from making any calls. The
> evaluate methods and setValue:forKey:. I can look at doing this.

Is this to prevent WebKit clients from calling into JavaScript? Or is it about WebKit plugins? I've made no attempts to block WebKit clients so far.

[Timothy Hatcher]

(In reply to comment #15)
> (In reply to comment #14)
> > On Mac we will need to prevent WebScriptObject from making any calls. The
> > evaluate methods and setValue:forKey:. I can look at doing this.
> 
> Is this to prevent WebKit clients from calling into JavaScript? Or is it about
> WebKit plugins? I've made no attempts to block WebKit clients so far.

Really both.

[Adam Roben (:aroben)]

Comment on attachment 19589 [details]
[WIP] Make it possible to pause/resume the DatabaseThread

After discussing with Alexey, I've realized that this patch is not sufficient. One problem with it is that it does not stop already-queued callbacks from executing.

To fix this completely Alexey and I think we'll need a way to pause all queued actions passed to callOnMainThread. This will require patching each platform's implementation of this function separately.

[Adam Roben (:aroben)]

Created attachment 19725 [details]
[WIP] Make it possible to pause/resume callOnMainThread callbacks (Windows only)

This patch replaces the DatabaseThread patch. It allows you to pause/resume callbacks scheduled through callOnMainThread, which is what the DatabaseThread uses to schedule transaction callbacks. This implementation is Windows-only, just to have a reference point for talking about this concept.

[Adam Roben (:aroben)]

Created attachment 19726 [details]
[WIP (revised)] Pause/resume of callOnMainThread (Windows only)

This revised patch removes the guard of callbacksPaused and adds a comment that setMainThreadCallbacksPaused must be called from the main thread. It also fixes a reversed assignment.

[Adam Roben (:aroben)]

Created attachment 19728 [details]
[WIP] Make more MainThread code cross-platform

This patch makes more of the MainThread code from the Windows implementation cross-platform so that it may be shared with other platforms.

[Adam Roben (:aroben)]

Created attachment 19729 [details]
[WIP (revised 2)] Pause/resume of callOnMainThread

New patch that relies on attachment 19728 [details]

[Adam Roben (:aroben)]

Created attachment 19730 [details]
[WIP (revised)] Make more MainThread code shareable

[Adam Roben (:aroben)]

Created attachment 19731 [details]
[WIP] Share most MainThread code across all platforms

This patch is missing a change to WebCore.xcodeproj to add MainThread.cpp to the project, but other than that (and need a ChangeLog) it should be ready to go.

[Adam Roben (:aroben)]

Comment on attachment 19729 [details]
[WIP (revised 2)] Pause/resume of callOnMainThread

This patch now works cross-platform (note that it depends on attachment 19731 [details]).

[Adam Roben (:aroben)]

Here are the ways I currently know that JavaScript execution can be initiated:

1. JavaScript event handlers
2. Callbacks from window.setTimeout/window.setInterval
3. Callbacks from the client-side database storage API
4. JavaScript access from plugins (both NPAPI and WebKit plugins)
5. Parsing of a <script> tag in a document
6. Loading of a script resource completing
7. Loading of javascript: URIs
8. Calls from WebKit clients

[Adam Roben (:aroben)]

(In reply to comment #25)
> Here are the ways I currently know that JavaScript execution can be initiated:
> 
> 1. JavaScript event handlers
> 2. Callbacks from window.setTimeout/window.setInterval

These are handled by attachment 19588 [details] (except for the FIXME about timers scheduled while paused).

> 3. Callbacks from the client-side database storage API

This is handled by attachment 19729 [details].

> 4. JavaScript access from plugins (both NPAPI and WebKit plugins)

This is handled for Windows by attachment 19590 [details]. Making this work on Mac will involve similar changes to WebBaseNetscapePluginView and blocking calls to WebScriptObject.

> 5. Parsing of a <script> tag in a document
> 6. Loading of a script resource completing

I believe these can be handled by calling Page::setDefersLoading(true).

> 7. Loading of javascript: URIs

I don't yet have a solution for this.

> 8. Calls from WebKit clients

I don't yet plan to try to block this, since for the moment I'm only concerned with getting this working for in-process JavaScript debugging in the Inspector (bug 17134), and I think we can get by without this for those purposes.

[Adam Roben (:aroben)]

Created attachment 19744 [details]
Make most of callOnMainThread be cross-platform

[Adam Roben (:aroben)]

(In reply to comment #26)
> (In reply to comment #25)
> > Here are the ways I currently know that JavaScript execution can be initiated:
> > 7. Loading of javascript: URIs
> 
> I don't yet have a solution for this.

I think I now understand how to deal with this.

Here are the ways a javascript: URI may be evaluated:

A. Parsing of a <frame>/<iframe> tag that has a javascript: URI as the value of its src attribute

   This can be prevented by Page::setDefersLoading(true), just as with parsing of <script> elements.

B. Programmatically changing window.location or the src attribute of a <frame>/<iframe> to a javascript: URI

   This will be prevented simply because we're going to be pausing JavaScript.

C. User activating an <a> that has a javascript: URI as the value of its href attribute
D. User submitting a form that has a javascript: URI as the value of its action attribute

   These we will need to block. Programmatic activation/submission (e.g., anchor.click()/form.submit()) should be allowed.

E. "Navigation" of the main frame to a javascript: URI by the WebKit client

   This falls under the general category of calls from WebKit clients that execute JavaScript (this is item (8) from comment 25). I'm not concerned with this right now.

So, all we need to do is make sure that the link's/form's javascript: URI is not evaluated if the user clicks a link/submits a form.

[Adam Roben (:aroben)]

(In reply to comment #28)
> B. Programmatically changing window.location or the src attribute of a
> <frame>/<iframe> to a javascript: URI
> 
>    This will be prevented simply because we're going to be pausing JavaScript.

I should be slightly clearer here. If the programmatic change is in the code that we are stepping through in the debugger, we will allow this to happen. If the programmatic change is in other code, it won't be executing because JavaScript will be paused.

[Alexey Proskuryakov]

Comment on attachment 19744 [details]
Make most of callOnMainThread be cross-platform

r=me

+				RelativePath="..\platform\MainThreadInternal.h"

I'm not sure if I love the file name "MainThreadInternal" - Internal has all sorts of connotations, which do not apply here :)

I think I'd just keep these functions in the same file, although either way is OK with me.

-    if (processingCustomThreadingMessage)
-        LOG(Threading, "callOnMainThread() called recursively.  Beware of nested PostMessage()s");
+    if (dispatchingCallbacks)
+        LOG(Threading, "callOnMainThread() called recursively.");

I do not understand this check. What's the problem with a thread making a callback when the main thread is working through previous ones (possibly from other threads)? I suggest removing it, if a reason doesn't come up.

[Adam Roben (:aroben)]

(In reply to comment #30)
> (From update of attachment 19744 [details] [edit])
> -    if (processingCustomThreadingMessage)
> -        LOG(Threading, "callOnMainThread() called recursively.  Beware of
> nested PostMessage()s");
> +    if (dispatchingCallbacks)
> +        LOG(Threading, "callOnMainThread() called recursively.");
> 
> I do not understand this check. What's the problem with a thread making a
> callback when the main thread is working through previous ones (possibly from
> other threads)? I suggest removing it, if a reason doesn't come up.

Hopefully Brady can shed some light here, since he wrote the code originally.

[Brady Eidson]

I'm fairly certain I modeled the original code after a public domain "Call a function on the main thread from any other thread in Windows" tutorial.

It's been awhile - I remember the concern of an infinite recursion into PostMessage, but don't remember details beyond that.  

If someone more familiar with the Windows message loop decides the concern is unwarranted in this application, fine by me to remove it!

[Adam Roben (:aroben)]

(In reply to comment #32)
> I'm fairly certain I modeled the original code after a public domain "Call a
> function on the main thread from any other thread in Windows" tutorial.
> 
> It's been awhile - I remember the concern of an infinite recursion into
> PostMessage, but don't remember details beyond that.  
> 
> If someone more familiar with the Windows message loop decides the concern is
> unwarranted in this application, fine by me to remove it!

I don't think there's a danger of recursion beneath PostMessage -- the whole point of PostMessage is that the message won't be dealt with until after we return to the message pump.

Thanks for the explanation!

[Adam Roben (:aroben)]

Created attachment 19768 [details]
[1/2] Make PausedTimeouts be internal to JSDOMWindowBase

[Adam Roben (:aroben)]

Created attachment 19769 [details]
[2/2] Remove PausedTimeouts

[Adam Roben (:aroben)]

Created attachment 19770 [details]
[WIP] Add KJSProxy::[set]JavaScriptExecutionAllowed to pause timeouts and block JS event handlers

This depends on attachment 19769 [details].

[Adam Roben (:aroben)]

Created attachment 19771 [details]
[WIP] Pause JS timers, block javascript: URIs, block JS event handlers

This patch adds KJSProxy::[set]JavaScriptExecutionAllowed. Calling setJavaScriptExecutionAllowed(false) will pause JS timers, block javascript: URIs, and block JS event handlers. This patch depends on attachment 19769 [details].

This takes care of items (1), (2), and (7) from comment 25.

[Adam Roben (:aroben)]

Comment on attachment 19744 [details]
Make most of callOnMainThread be cross-platform

Committed in r31063

[Geoffrey Garen]

Comment on attachment 19771 [details]
[WIP] Pause JS timers, block javascript: URIs, block JS event handlers

I think setPaused / isPaused would be a better naming scheme than setJavaScriptExecutionAllowed  / javaScriptExecutionAllowed. It matches the style of setEnabled / isEnabled, it's more terse, and it's more specific about why JavaScript execution is not allowed, and what will happen to attempts to execute JavaScript.

+void KJSProxy::setJavaScriptExecutionAllowed(bool allowed)
+{
+    if (!m_globalObject)
+        return;
+

This code seems wrong. It means that a proxy that didn't have a global object wouldn't "get the memo" about JavaScript being paused, so it might start up JS execution at a later time.

r=me, with comments

[Geoffrey Garen]

Comment on attachment 19768 [details]
[1/2] Make PausedTimeouts be internal to JSDOMWindowBase

You need to hand off the paused timeouts to the CachedPage, otherwise they can't resume when the CachedPage is displayed again.

[Geoffrey Garen]

Comment on attachment 19769 [details]
[2/2] Remove PausedTimeouts

r=me

[Adam Roben (:aroben)]

(In reply to comment #40)
> (From update of attachment 19768 [details] [edit])
> You need to hand off the paused timeouts to the CachedPage, otherwise they
> can't resume when the CachedPage is displayed again.

A little clarification for those following along at home: the JSDOMWindowBase object is reused for each navigation, and the CachedPage's job is to save off information from JSDOMWindowBase (and other objects) that needs to be restored later.

[Adam Roben (:aroben)]

Comment on attachment 19771 [details]
[WIP] Pause JS timers, block javascript: URIs, block JS event handlers

After some more thinking, I've realized we don't need to worry about new timers being installed while JavaScript is paused. The timers can only be installed programmatically, which can't happen while JavaScript is paused.

So, the PausedTimeouts refactoring is not necessary, and I'll post a newer version of this patch.

[Adam Roben (:aroben)]

Created attachment 19788 [details]
[WIP] Block javascript: URIs and JS event handlers while paused

[Adam Roben (:aroben)]

Created attachment 19915 [details]
Block JS event handlers/javascript: URIs per-Frame

[Adam Roben (:aroben)]

Created attachment 19916 [details]
Pause callOnMainThread callbacks

[Adam Roben (:aroben)]

Created attachment 19917 [details]
Pause/block JS execution from plugins

[Adam Roben (:aroben)]

Comment on attachment 19915 [details]
Block JS event handlers/javascript: URIs per-Frame

Committed in r31197

[Adam Roben (:aroben)]

Comment on attachment 19916 [details]
Pause callOnMainThread callbacks

Committed in r31198

[Adam Roben (:aroben)]

Comment on attachment 19917 [details]
Pause/block JS execution from plugins

Committed in r31199

[Adam Roben (:aroben)]

Created attachment 19926 [details]
Don't allow newly-scheduled plugin requests to be serviced while JS is paused

[Adam Roben (:aroben)]

Comment on attachment 19926 [details]
Don't allow newly-scheduled plugin requests to be serviced while JS is paused

Committed in r31210

[Alexey Proskuryakov]

Is this still something we want (and haven't done yet)?

[Adam Roben (:aroben)]

I'm pretty sure this task was finished (or finished enough to let the Inspector's JS debugger work) when the last patch landed, and I just forgot to close the bug.