Bug 167457 – Web Inspector: allow import() inside the inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Yusuke Suzuki]

We need to add ScriptFetcher for inspector's script execution after WebCore dynamic-import patch is landed.

[Joseph Pecoraro]

What does this bug mean to "allow import()". Allow it where? In the Inspector's console? In the Web Inspector code itself?

[Yusuke Suzuki]

(In reply to comment #2)
> What does this bug mean to "allow import()". Allow it where? In the
> Inspector's console? In the Web Inspector code itself?

In the inspector's console. The goal is evaluating `await import("...")`.
It will import the module and return the module namespace object!

This is blocked by 2 issues.

1. This issue. Currently, we do not set effective ScriptFetcher for the code executed from the inspector console. This should be fixed in this issue. This allows us to evaluate `import()` in the console. Currently, it always returns rejected promise.

2. Accepting `await import()`. This is now syntax error since Esprima does not recognize `import()` new operator.

[Joseph Pecoraro]

Awesome!

[Yusuke Suzuki]

Related links for Esprima supports.

1. https://github.com/estree/estree/pull/137 AST node PR
2. https://github.com/jquery/esprima/issues/1728 Esprima issue.

[Radar WebKit Bug Importer]

<rdar://problem/30249567>

[Yusuke Suzuki]

Created attachment 300335 [details]
Patch

WIP

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


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

[Yusuke Suzuki]

Created attachment 300397 [details]
Patch

[Blaze Burg]

Comment on attachment 300397 [details]
Patch

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

Test looks good with some nits. I would like someone more familiar with module code to review JSC changes.

> LayoutTests/inspector/controller/runtime-controller.html:134
> +                    callback(result, wasThrown);

Shouldn't you be logging wasThrown and the exception value if it threw one somewhere? This is currently only testing that it goes through console.log, not as return value from evaluateInInspectedWindow().

> LayoutTests/inspector/controller/runtime-controller.html:138
> +            function expectUndefined(result, wasThrown) {

I think you can just inline this into testSource(), we only ever use this function for `callback` parameter.

[Yusuke Suzuki]

Comment on attachment 300397 [details]
Patch

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

Thanks!

>> LayoutTests/inspector/controller/runtime-controller.html:134
>> +                    callback(result, wasThrown);
> 
> Shouldn't you be logging wasThrown and the exception value if it threw one somewhere? This is currently only testing that it goes through console.log, not as return value from evaluateInInspectedWindow().

`result` always become undefined if we use `await` in console. And wasThrow is always false. Thrown error is drained by the try-catch in the rewrote code for `await`.

>> LayoutTests/inspector/controller/runtime-controller.html:138
>> +            function expectUndefined(result, wasThrown) {
> 
> I think you can just inline this into testSource(), we only ever use this function for `callback` parameter.

OK, done.

[Yusuke Suzuki]

Created attachment 300531 [details]
Patch

[Yusuke Suzuki]

Created attachment 300532 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 300532 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:225
> +    //     1. The code evaluated by the inspector.
> +    //     2. The other unusual code execution like the evaluation through the NPAPI.

Other cases to consider will be extension scripts & injected bundle's scripts.

[Yusuke Suzuki]

Comment on attachment 300532 [details]
Patch

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

>> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:225
>> +    //     2. The other unusual code execution like the evaluation through the NPAPI.
> 
> Other cases to consider will be extension scripts & injected bundle's scripts.

Thanks! I've opened the bug for that. https://bugs.webkit.org/show_bug.cgi?id=167926

[Yusuke Suzuki]

Committed r211777: <http://trac.webkit.org/changeset/211777>

[Ryan Haddad]

(In reply to comment #16)
> Committed r211777: <http://trac.webkit.org/changeset/211777>

The LayoutTest for this change appears to be failing: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r211811%20(11376)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcontroller%2Fruntime-controller.html

[Yusuke Suzuki]

Committed r211818: <http://trac.webkit.org/changeset/211818>

[Yusuke Suzuki]

(In reply to comment #17)
> (In reply to comment #16)
> > Committed r211777: <http://trac.webkit.org/changeset/211777>
> 
> The LayoutTest for this change appears to be failing:
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r211811%20(11376)/results.html
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Fcontroller%2Fruntime-controller.html

It seems that this is because the added tests are not deterministic.
I'll update/add the deterministic one.
I've just manually rolled out the patch.

[Yusuke Suzuki]

Reopen the bug to add the deterministic tests.

[Yusuke Suzuki]

Committed r212438: <http://trac.webkit.org/changeset/212438>

[Yusuke Suzuki]

Reland it with the deterministic tests extracted from the original test file.

[Yusuke Suzuki]

(In reply to comment #22)
> Reland it with the deterministic tests extracted from the original test file.

Hmmm, still failing. But the test works fine in GTK port. Looking...

[Yusuke Suzuki]

Committed r212444: <http://trac.webkit.org/changeset/212444>