Bug 167457

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

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

(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.

Awesome!

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.

<rdar://problem/30249567>

Created attachment 300335 [details] Patch WIP

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.

Created attachment 300397 [details] Patch

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 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.

Created attachment 300531 [details] Patch

Created attachment 300532 [details] Patch

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 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

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

(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

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

(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.

Reopen the bug to add the deterministic tests.

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

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

(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...

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