Bug 167457 - Web Inspector: allow import() inside the inspector
Summary: Web Inspector: allow import() inside the inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 166926 167698
Blocks: 167926
  Show dependency treegraph
 
Reported: 2017-01-26 08:21 PST by Yusuke Suzuki
Modified: 2017-02-16 10:22 PST (History)
10 users (show)

See Also:


Attachments
Patch (5.90 KB, patch)
2017-02-01 10:04 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.66 KB, patch)
2017-02-02 03:30 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2017-02-03 07:11 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2017-02-03 07:13 PST, Yusuke Suzuki
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-01-26 08:21:33 PST
...
Comment 1 Yusuke Suzuki 2017-01-26 22:26:55 PST
We need to add ScriptFetcher for inspector's script execution after WebCore dynamic-import patch is landed.
Comment 2 Joseph Pecoraro 2017-01-26 22:53:39 PST
What does this bug mean to "allow import()". Allow it where? In the Inspector's console? In the Web Inspector code itself?
Comment 3 Yusuke Suzuki 2017-01-27 01:11:46 PST
(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.
Comment 4 Joseph Pecoraro 2017-01-27 10:48:28 PST
Awesome!
Comment 5 Yusuke Suzuki 2017-01-28 02:32:24 PST
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.
Comment 6 Radar WebKit Bug Importer 2017-01-28 13:30:27 PST
<rdar://problem/30249567>
Comment 7 Yusuke Suzuki 2017-02-01 10:04:19 PST
Created attachment 300335 [details]
Patch

WIP
Comment 8 WebKit Commit Bot 2017-02-01 10:21:30 PST
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.
Comment 9 Yusuke Suzuki 2017-02-02 03:30:13 PST
Created attachment 300397 [details]
Patch
Comment 10 Brian Burg 2017-02-02 09:50:36 PST
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.
Comment 11 Yusuke Suzuki 2017-02-03 07:10:57 PST
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.
Comment 12 Yusuke Suzuki 2017-02-03 07:11:52 PST
Created attachment 300531 [details]
Patch
Comment 13 Yusuke Suzuki 2017-02-03 07:13:27 PST
Created attachment 300532 [details]
Patch
Comment 14 Ryosuke Niwa 2017-02-06 22:54:23 PST
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.
Comment 15 Yusuke Suzuki 2017-02-07 00:15:20 PST
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
Comment 16 Yusuke Suzuki 2017-02-07 00:18:51 PST
Committed r211777: <http://trac.webkit.org/changeset/211777>
Comment 18 Yusuke Suzuki 2017-02-07 10:26:27 PST
Committed r211818: <http://trac.webkit.org/changeset/211818>
Comment 19 Yusuke Suzuki 2017-02-07 10:28:06 PST
(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.
Comment 20 Yusuke Suzuki 2017-02-07 10:29:06 PST
Reopen the bug to add the deterministic tests.
Comment 21 Yusuke Suzuki 2017-02-16 09:33:56 PST
Committed r212438: <http://trac.webkit.org/changeset/212438>
Comment 22 Yusuke Suzuki 2017-02-16 09:34:30 PST
Reland it with the deterministic tests extracted from the original test file.
Comment 23 Yusuke Suzuki 2017-02-16 10:16:07 PST
(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...
Comment 24 Yusuke Suzuki 2017-02-16 10:22:09 PST
Committed r212444: <http://trac.webkit.org/changeset/212444>