Bug 134371 – JSContext Inspection: Provide a way to use a non-Main RunLoop for Inspector JavaScript Evaluations

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Some JSContext applications assert that JavaScript executes on a particular dispatch_queue or RunLoop. If a JSContext inspector is opened, and is used to call functions from its quick console, it would evaluate that in the JSContext on the main run loop. This would cause these applications to assert, and generally makes debugging difficult.

Currently, Web Inspector requires a run loop for pausing.

We should allow an application to provide a RunLoop which the debugger can use to evaluate JavaScript on, and not unnecessarily assert by using an unexpected run loop.

[Joseph Pecoraro]

<rdar://problem/16392363>

[Joseph Pecoraro]

Created attachment 233949 [details]
[PATCH] Proposed Fix

I tested this on OS X and iOS with a simple test application to check edge conditions. Everything behaved as I expected. I want to do some more testing on this, and maybe include an updated patch with some basic JSC tests for the property.

Unfortunately there is some duplicated code in this patch between the global shared path and this debuggable specific path. I was not sure how best to share the code that didn't just look uglier.

[WebKit Commit Bot]

Attachment 233949 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSContextRef.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:144:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Comment on attachment 233949 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=233949&action=review

Looks good.

> Source/JavaScriptCore/API/JSContextPrivate.h:51
> +@property (setter=_setDebuggerRunLoop:) CFRunLoopRef _debuggerRunLoop NS_AVAILABLE(10_10, 8_0);

It is annoying this needs to use a C type in ObjC. But at least NSRunLoop has a getter for CFRunLoopRef.

I would say make this use NSRunLoop and have the implementation for _setDebuggerRunLoop: translate to a CFRunLoopRef. But there is no way to turn a CFRunLoopRef into a NSRunLoop.

[Joseph Pecoraro]

Comment on attachment 233949 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=233949&action=review

>> Source/JavaScriptCore/API/JSContextPrivate.h:51
>> +@property (setter=_setDebuggerRunLoop:) CFRunLoopRef _debuggerRunLoop NS_AVAILABLE(10_10, 8_0);
> 
> It is annoying this needs to use a C type in ObjC. But at least NSRunLoop has a getter for CFRunLoopRef.
> 
> I would say make this use NSRunLoop and have the implementation for _setDebuggerRunLoop: translate to a CFRunLoopRef. But there is no way to turn a CFRunLoopRef into a NSRunLoop.

Correct. I think its better to have a property than a one way. If we made this NSRunLoop, we could just assume nobody will change the run loop out from under the ObjC API using the C API.

[Joseph Pecoraro]

Created attachment 233999 [details]
[PATCH] For Bots 1

[WebKit Commit Bot]

Attachment 233999 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSContextRef.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:145:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 234002 [details]
[PATCH] For Bots 2

[Joseph Pecoraro]

Created attachment 234006 [details]
[PATCH] For Bots 3

[Joseph Pecoraro]

Comment on attachment 234006 [details]
[PATCH] For Bots 3

View in context: https://bugs.webkit.org/attachment.cgi?id=234006&action=review

> Source/JavaScriptCore/API/JSContextRefPrivate.h:150
> +#if USE(CF)
> +/*!
> +@function
> +@abstract Gets the run loop used by the Web Inspector debugger when evaluating JavaScript in this context.
> +@param ctx The JSGlobalContext whose setting you want to get.
> +*/
> +JS_EXPORT CFRunLoopRef JSGlobalContextGetDebuggerRunLoop(JSGlobalContextRef ctx) CF_AVAILABLE(10_10, 8_0);
> +
> +/*!
> +@function
> +@abstract Sets the run loop used by the Web Inspector debugger when evaluating JavaScript in this context.
> +@param ctx The JSGlobalContext that you want to change.
> +@param runLoop The new value of the setting for the context.
> +*/
> +JS_EXPORT void JSGlobalContextSetDebuggerRunLoop(JSGlobalContextRef ctx, CFRunLoopRef) CF_AVAILABLE(10_10, 8_0);
> +#endif

Using USE(CF) inside of a Private header is not going to fly. I don't think there is any clean way to make this clean in the private header.

Lets make this an ObjC only SPI. Move the C API bits to an Internal header. Maybe than I can switch to NSRunLoop.

[Joseph Pecoraro]

Created attachment 234016 [details]
[PATCH] For Bots 4

[Joseph Pecoraro]

r170589: <http://trac.webkit.org/changeset/170589>