Bug 122764 – Web Inspector: Avoid using Runtime.executionContextCreated to figure out the iframe's contentDocument node.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Runtime.executionContextCreated is only called if you have a <script> tag in the page, so iframes with no scripting will not show up in the WebInspector.

Also, it means that the console will not be able to inject any other script inside the iframe, so maybe we just need to always create a scripting context when the web inspector is loaded.

Note that it happens to work in Safari because of the do-not-track feature that will always create an exceution context for all pages. However, that's not the case for the test runner.
The context is created because of the following function call Safari::BrowserBundlePageController::injectDoNotTrackDOMPropertyInStandardWorld().

[Radar WebKit Bug Importer]

<rdar://problem/15222136>

[Alexandru Chiculita]

I've logged https://bugs.webkit.org/show_bug.cgi?id=122766 for the do-not-track issue.

[Yury Semikhatsky]

Created attachment 384850 [details]
Patch

[Yury Semikhatsky]

Created attachment 384854 [details]
Patch

[Devin Rousso]

Comment on attachment 384854 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:695
> +    mainWorldExecState(&frame);

Why is this being done inside the page agent?  Why create a `PageDebuggerAgent::frameNavigated` (or change the existing `PageDebuggerAgent::mainFrameNavigated` to be that and have a check inside it for `frame.isMainFrame()`)?

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:5
> +function createIFrame()

NIT: I'd just call this `createFrame`.

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:14
> +    let suite = ProtocolTest.createAsyncSuite("Initial Runtime.executionContextCreated events");

NIT: we prefer to have suite names that would match object property syntax.
```
    let suite = ProtocolTest.createAsyncSuite("Runtime.executionContextCreated.ContextWithoutScript");
```

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:17
> +        name: "No duplicate contexts when enabling Runtime agent",

NIT: we prefer to use the suite name as a prefix for test case names.
```
    name: "Runtime.executionContextCreated.ContextWithoutScript.SubFrame",
```
Also, what you have here doesn't match what the test is actually doing.

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:18
> +        description: "Test that Runtime.enable will send executionContextCreated events only for existng main world contexts",

This doesn't match what the test is actually doing.

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:21
> +                method: "Page.enable"

Style: for simple objects like this (1-2 keys with primitive or reference values), we usually inline them.

```
    await InspectorProtocol.awaitCommand({method: "Page.enable"});
    await InspectorProtocol.awaitCommand({method: "Runtime.enable"});
```

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:31
> +            });

Style: please add a newline after

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:32
> +            await InspectorProtocol.awaitEvent({event: "Runtime.executionContextCreated"});

In cases like these, I usually prefer to add the event listener _before_ I dispatch the command that would trigger the event, especially when using `await`, so that it's guaranteed to be handled if execution is slow, such as in a debug build.
```
    await Promise.all([
        InspectorProtocol.awaitEvent({event: "Runtime.executionContextCreated"}),
        InspectorProtocol.awaitCommand({
            method: "Runtime.enable",
            params: {expression: `createFrame()`},
        }),
    ]);
```

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:33
> +            ProtocolTest.pass("Should receive Runtime.executionContextCreated notification");

We should also test that we're able to do something with the newly created execution context, like get the `document.body.textContent` and check that it matches "No JavaScript".

Style: please add a newline before

[Yury Semikhatsky]

Comment on attachment 384854 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:695
>> +    mainWorldExecState(&frame);
> 
> Why is this being done inside the page agent?  Why create a `PageDebuggerAgent::frameNavigated` (or change the existing `PageDebuggerAgent::mainFrameNavigated` to be that and have a check inside it for `frame.isMainFrame()`)?

We cannot put this into a debugger agent because executionContextCreated event is a part of Runtime domain (not debugger) and it is generated by PageRuntimeAgent. PageRuntimeAgent requires InspectorPageAgent to be enabled and does not require debugger to be on.

I've added PageRuntimeAgent::frameNavigated instead and moved the logic over there as PageRuntimeAgent is the place where execution contexts are managed.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:5
>> +function createIFrame()
> 
> NIT: I'd just call this `createFrame`.

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:14
>> +    let suite = ProtocolTest.createAsyncSuite("Initial Runtime.executionContextCreated events");
> 
> NIT: we prefer to have suite names that would match object property syntax.
> ```
>     let suite = ProtocolTest.createAsyncSuite("Runtime.executionContextCreated.ContextWithoutScript");
> ```

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:17
>> +        name: "No duplicate contexts when enabling Runtime agent",
> 
> NIT: we prefer to use the suite name as a prefix for test case names.
> ```
>     name: "Runtime.executionContextCreated.ContextWithoutScript.SubFrame",
> ```
> Also, what you have here doesn't match what the test is actually doing.

Updated the text.

What's the point in repeating test harness name multiple times? IMO it just hinders readability of the test output. Also if we want all test cases to be prefixed by the harness name, the test harness implementation could add the prefix automatically in the output.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:18
>> +        description: "Test that Runtime.enable will send executionContextCreated events only for existng main world contexts",
> 
> This doesn't match what the test is actually doing.

Updated.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:21
>> +                method: "Page.enable"
> 
> Style: for simple objects like this (1-2 keys with primitive or reference values), we usually inline them.
> 
> ```
>     await InspectorProtocol.awaitCommand({method: "Page.enable"});
>     await InspectorProtocol.awaitCommand({method: "Runtime.enable"});
> ```

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:31
>> +            });
> 
> Style: please add a newline after

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:32
>> +            await InspectorProtocol.awaitEvent({event: "Runtime.executionContextCreated"});
> 
> In cases like these, I usually prefer to add the event listener _before_ I dispatch the command that would trigger the event, especially when using `await`, so that it's guaranteed to be handled if execution is slow, such as in a debug build.
> ```
>     await Promise.all([
>         InspectorProtocol.awaitEvent({event: "Runtime.executionContextCreated"}),
>         InspectorProtocol.awaitCommand({
>             method: "Runtime.enable",
>             params: {expression: `createFrame()`},
>         }),
>     ]);
> ```

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:33
>> +            ProtocolTest.pass("Should receive Runtime.executionContextCreated notification");
> 
> We should also test that we're able to do something with the newly created execution context, like get the `document.body.textContent` and check that it matches "No JavaScript".
> 
> Style: please add a newline before

Done.

[Yury Semikhatsky]

Created attachment 384918 [details]
Patch

[Devin Rousso]

Comment on attachment 384854 [details]
Patch

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

>>> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:695
>>> +    mainWorldExecState(&frame);
>> 
>> Why is this being done inside the page agent?  Why create a `PageDebuggerAgent::frameNavigated` (or change the existing `PageDebuggerAgent::mainFrameNavigated` to be that and have a check inside it for `frame.isMainFrame()`)?
> 
> We cannot put this into a debugger agent because executionContextCreated event is a part of Runtime domain (not debugger) and it is generated by PageRuntimeAgent. PageRuntimeAgent requires InspectorPageAgent to be enabled and does not require debugger to be on.
> 
> I've added PageRuntimeAgent::frameNavigated instead and moved the logic over there as PageRuntimeAgent is the place where execution contexts are managed.

Ah whoops.  I meant to write `PageRuntimeAgent` 😂  Glad you got it nonetheless :)

>>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:17
>>> +        name: "No duplicate contexts when enabling Runtime agent",
>> 
>> NIT: we prefer to use the suite name as a prefix for test case names.
>> ```
>>     name: "Runtime.executionContextCreated.ContextWithoutScript.SubFrame",
>> ```
>> Also, what you have here doesn't match what the test is actually doing.
> 
> Updated the text.
> 
> What's the point in repeating test harness name multiple times? IMO it just hinders readability of the test output. Also if we want all test cases to be prefixed by the harness name, the test harness implementation could add the prefix automatically in the output.

Using object property syntax helps with code searching, especially when using terminal commands like `grep` or `ack` where spaces can be treated differently.  We like to use the suite name as a prefix for a similar reason, and would also help if we ever decided to add something like a per-testcase dashboard, rather than just per-file, as an individual test case would contain both where it is (the suite prefix) and what it does (the testcase name).

[Devin Rousso]

Comment on attachment 384918 [details]
Patch

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

r=me, with some style comments

> Source/WebCore/inspector/InspectorInstrumentation.cpp:720
> +    if (PageRuntimeAgent* pageRuntimeAgent = instrumentingAgents.pageRuntimeAgent())

NIT: `auto`

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:22
> +            await InspectorProtocol.awaitCommand({method: "Runtime.enable"});

Style: please add a newline after

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:23
> +            let [eventPayload] = await Promise.all([

Style: please use a more descriptive name, like `executionContextCreatedEvent`

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:27
> +                    params: {expression: "createFrame()"}

Style: we usually use backticks (`) for expressions that will be evaluated in the inspected page, so it's more distinct and can be more easily identified when scanning a file

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:33
> +            let resultPayload = await InspectorProtocol.awaitCommand({

Style: please use a more descriptive name, like `evaluateResult`

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:37
> +                    expression: "document.body.textContent"

Ditto (27)

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:41
> +            ProtocolTest.expectEqual(resultPayload.result.value, "No JavaScript.", "Can evaluate in the subframe using new context.");

Style: our usual style is to make the expectation message an imperative sentence, like "Should be able to evaluate in subframe."

[Yury Semikhatsky]

Comment on attachment 384854 [details]
Patch

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

>>>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:17
>>>> +        name: "No duplicate contexts when enabling Runtime agent",
>>> 
>>> NIT: we prefer to use the suite name as a prefix for test case names.
>>> ```
>>>     name: "Runtime.executionContextCreated.ContextWithoutScript.SubFrame",
>>> ```
>>> Also, what you have here doesn't match what the test is actually doing.
>> 
>> Updated the text.
>> 
>> What's the point in repeating test harness name multiple times? IMO it just hinders readability of the test output. Also if we want all test cases to be prefixed by the harness name, the test harness implementation could add the prefix automatically in the output.
> 
> Using object property syntax helps with code searching, especially when using terminal commands like `grep` or `ack` where spaces can be treated differently.  We like to use the suite name as a prefix for a similar reason, and would also help if we ever decided to add something like a per-testcase dashboard, rather than just per-file, as an individual test case would contain both where it is (the suite prefix) and what it does (the testcase name).

The expectation could include full path which would help with both grepping and dashboards (for the dashboards the harness could assist too). Requiring to repeat the same constant multiple times is error-prone to (it's easy to make a typo). Anyways, consistency across the tests is important. I'd personally prefer BDD-style test definitions as it would also allow to get rid of separate description and it seems more readable, it has nothing to do with this patch though.

[Yury Semikhatsky]

Comment on attachment 384918 [details]
Patch

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

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:720
>> +    if (PageRuntimeAgent* pageRuntimeAgent = instrumentingAgents.pageRuntimeAgent())
> 
> NIT: `auto`

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:22
>> +            await InspectorProtocol.awaitCommand({method: "Runtime.enable"});
> 
> Style: please add a newline after

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:23
>> +            let [eventPayload] = await Promise.all([
> 
> Style: please use a more descriptive name, like `executionContextCreatedEvent`

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:27
>> +                    params: {expression: "createFrame()"}
> 
> Style: we usually use backticks (`) for expressions that will be evaluated in the inspected page, so it's more distinct and can be more easily identified when scanning a file

Done. It could be misleading though as backticks are also used for template strings.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:33
>> +            let resultPayload = await InspectorProtocol.awaitCommand({
> 
> Style: please use a more descriptive name, like `evaluateResult`

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:37
>> +                    expression: "document.body.textContent"
> 
> Ditto (27)

Done.

>> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:41
>> +            ProtocolTest.expectEqual(resultPayload.result.value, "No JavaScript.", "Can evaluate in the subframe using new context.");
> 
> Style: our usual style is to make the expectation message an imperative sentence, like "Should be able to evaluate in subframe."

Done.

[Yury Semikhatsky]

Created attachment 384925 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384925 [details]
Patch for landing

Clearing flags on attachment: 384925

Committed r253166: <https://trac.webkit.org/changeset/253166>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.