Bug 147942 – Add InspectorInstrumentation builtin object to instrument the code in JS builtins like Promises

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

[Yusuke Suzuki]

To provide information to the inspector, JS builtins should have some sort of instrumentation system.
In this patch, we'll introduce a new private namespace object (like Reflect/Math) @InspectorInstrumentation into the builtin JS.
It gives the instrumentation ability to the builtin JS code in JSC.

Here's my design.
In @fulfillPromise in Operation.Promise.js,

function fulfillPromise(...)
{
    ...
    @InspectorInstrumentation.promiseFulfilled(promise);
    ...
}

And in InspectorInstrumentationObject.js (builtin JS code),

function promiseFulfilled(promise)
{
    if (!this.isEnabled)
        return;
    this.promiseFulfilledImpl(promise);  // call C++ code / other inspector code
}

[Yusuke Suzuki]

Created attachment 258834 [details]
Patch

Proof of Concept

[Yusuke Suzuki]

Created attachment 258835 [details]
Patch

Proof of Concept, with fix

[Blaze Burg]

Comment on attachment 258835 [details]
Patch

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

Looking pretty awesome so far!

> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:52
> +    promiseRejected   inspectorInstrumentationObjectPromiseRejected  DontEnum|Function 3

Do these need to be in the table, even if they are defined inside InspectorImplementationObject.js?

> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:65
> +    putDirectWithoutTransition(vm, Identifier::fromString(&vm, "isEnabled"), jsBoolean(false));

Is this InspectorInstrumentation object is instantiated once per context, per VM, or per page? Either way is fine, just need to know what objects we would need to find and set isEnabled for. We usually control whether inspector is enabled or not on a per-Page basis.

[Yusuke Suzuki]

Comment on attachment 258835 [details]
Patch

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

Later, I'll update the build fixed patch.

>> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:52
>> +    promiseRejected   inspectorInstrumentationObjectPromiseRejected  DontEnum|Function 3
> 
> Do these need to be in the table, even if they are defined inside InspectorImplementationObject.js?

Currently yes. InspectorImplementationObject.js just define the code. Picking up the function and set it as the method is the role of this table.
For example, Operations.Promise.js has the several functions which are not used as the methods of some objects (like rejectPromise).

If C++ or JS code becomes so typical, generating code by writing the new script may be nice.

>> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:65
>> +    putDirectWithoutTransition(vm, Identifier::fromString(&vm, "isEnabled"), jsBoolean(false));
> 
> Is this InspectorInstrumentation object is instantiated once per context, per VM, or per page? Either way is fine, just need to know what objects we would need to find and set isEnabled for. We usually control whether inspector is enabled or not on a per-Page basis.

Currently, InspectorInstrumentation object is instantiated once per JSGlobalObject (so, window object in WebCore). So, it is per page I think.
And if there's the iframe that has the different global object, it has the different InspectorInstrumentation object.

[Yusuke Suzuki]

Created attachment 258839 [details]
Patch

Proof of Concept, with fix, part 2

[Yusuke Suzuki]

Comment on attachment 258835 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:510
> +        GlobalPropertyInfo(vm.propertyNames->builtinNames().InspectorInstrumentationPrivateName(), InspectorInstrumentationObject::create(vm, this, InspectorInstrumentationObject::createStructure(vm, this, m_objectPrototype.get())), DontEnum | DontDelete | ReadOnly),

Here, we create the InspectorImplementation object and inject it into the JSGlobalObject's property (so it becomes the global variable with the private symbol name.)

[Yusuke Suzuki]

Created attachment 258867 [details]
Patch

[Yusuke Suzuki]

Created attachment 258885 [details]
Patch

[Blaze Burg]

Comment on attachment 258885 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        This patch adds new private global object, @InspectorInstruments.

typo.

[Yusuke Suzuki]

Created attachment 258896 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 258885 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch adds new private global object, @InspectorInstruments.
> 
> typo.

Thank you. Fixed.

[Blaze Burg]

Inspector parts look good, would like a JSC reviewer to peek as well.

[Geoffrey Garen]

Comment on attachment 258896 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:49
> +    debug             inspectorInstrumentationObjectDebug            DontEnum|Function 1

Can we just have log, without debug? They seem to be the same.

> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:50
> +    dataLogImpl       inspectorInstrumentationObjectDataLogImpl      DontEnum|Function 1

Let's call this "log".

[Yusuke Suzuki]

Comment on attachment 258896 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:49
>> +    debug             inspectorInstrumentationObjectDebug            DontEnum|Function 1
> 
> Can we just have log, without debug? They seem to be the same.

Make sense. I'll drop this function.

>> Source/JavaScriptCore/runtime/InspectorInstrumentationObject.cpp:50
>> +    dataLogImpl       inspectorInstrumentationObjectDataLogImpl      DontEnum|Function 1
> 
> Let's call this "log".

And rename it to the "log" :)

[Yusuke Suzuki]

Committed r188497: <http://trac.webkit.org/changeset/188497>