Permalink
Browse files

Fix module script error handling, again

Recent investigation determined more problems with the module script
graph fetching algorithms with regard to error handling. Essentially,
they were not robust to network races during fetching. Certain errors
would be propagated nondeterministically; worse, Evaluate() could be
called on modules with errored dependencies, due to another module
script graph interleaving its instantiation between a graph's
instantiation and evaluation.

This revision avoids all these problems. The strategy is essentially:

* Do not record instantiation errors of dependencies. This is mostly a
  feature of a corresponding revision to the JavaScript specification;
  we just no longer look for such errors. Instead, we call
  Instantiate(), which now does a from-scratch re-instantiation attempt,
  finding the appropriate error each time.
* Do not record parse errors of dependencies. Instead, traverse the
  graph right before instantiation to deterministically find the first
  parse error.

To implement this strategy, a new error-to-rethrow item was added to the
script struct, whose purpose is to store an error that is to be re-thrown
during instantiation. (An alternate implementation would have been to
find another way of passing the error from "prepare a script" and its
associated script-fetching algorithms, to "execute a script block" and
its associated script-running algorithms. But the script struct was a
convenient place to keep this.)

Note that with this strategy we no longer inspect the [[Status]] or
[[ErrorCompletion]] fields of JavaScript's Source Text Module Records;
instead, we can simply call Instantiate() or Evaluate() and deal with
any thrown errors. This is a much nicer encapsulation boundary.

This follows upon
tc39/ecma262@c0e07a8.
  • Loading branch information...
domenic committed Aug 30, 2017
1 parent eb17128 commit 165101a955652f715e551917c80ab8140429978f
Showing with 118 additions and 103 deletions.
  1. +118 −103 source
View
221 source
@@ -86172,7 +86172,19 @@ interface <dfn>NavigatorOnLine</dfn> {
error</dfn></dt>
<dd><p>A JavaScript value, which has meaning only if the <span
data-x="concept-script-record">record</span> is null.</p></dd>
data-x="concept-script-record">record</span> is null, indicating that the corresponding script
source text could not be parsed.</p></dd>
<dt>An <dfn data-dfn-for="script" data-export="" data-x="concept-script-error-to-rethrow">error
to rethrow</dfn></dt>
<dd>
<p>A JavaScript value representing an error that will prevent evaluation from succeeding. It
will be re-thrown by any attempts to <a href="#calling-scripts">run</a> the script.</p>
<p class="note">Since this exception value is provided by the JavaScript specification, we know
that it is never null, so we use null to signal that no error has occurred.</p>
</dd>
<dt><dfn data-dfn-for="script" data-export=""
data-x="concept-script-script-fetch-options">Fetch options</dfn></dt>
@@ -86206,41 +86218,6 @@ interface <dfn>NavigatorOnLine</dfn> {
data-x="concept-script">script</span>. It has no additional <span data-x="struct
item">items</span>.</p>
<p>We say that a <span>module script</span> <dfn data-x="concept-module-script-is-errored">is
errored</dfn> if either its <span data-x="concept-script-record">record</span> is null, or its
<span data-x="concept-script-record">record</span>'s [[Status]] field has the value "<code
data-x="">errored</code>".</p>
<p>When a <span>module script</span> <span data-x="concept-module-script-is-errored">is
errored</span>, we say that its <dfn data-x="concept-module-script-error">error</dfn> is either
its <span data-x="concept-script-parse-error">parse error</span>, when its <span
data-x="concept-script-record">record</span> is null, or its <span
data-x="concept-script-record">record</span>'s [[ErrorCompletion]] field's [[Value]] field,
otherwise.</p>
<p>To <dfn data-x="concept-module-script-set-pre-instantiation-error">set the pre-instantiation
error</dfn> of a <span>module script</span> <var>script</var>:</p>
<ol>
<li><p>Let <var>moduleRecord</var> be <var>script</var>'s <span
data-x="concept-script-record">record</span>.</p></li>
<li><p>If <var>moduleRecord</var> is not null, set <var>moduleRecord</var>.[[HostDefined]] to
undefined.</p></li>
<li><p>Set <var>script</var>'s <span data-x="concept-script-record">record</span> to
null.</p></li>
<li><p>Set <var>script</var>'s <span
data-x="concept-script-parse-error">parse error</span> to <var>error</var>.</p></li>
</ol>
<p>We say that a <span>module script</span> <dfn
data-x="concept-module-script-has-instantiated">has instantiated</dfn> if its <span
data-x="concept-script-record">record</span> is not null, and its <span
data-x="concept-script-record">record</span>'s [[Status]] field is either "<code
data-x="">instantiated</code>" or "<code data-x="">evaluated</code>".</p>
<hr>
<p>An <dfn data-export="">environment</dfn> is an object that identifies the settings of a
@@ -86757,12 +86734,8 @@ interface <dfn>NavigatorOnLine</dfn> {
module script">fetching a single module script</span> asynchronously completes with
<var>result</var>:</p></li>
<li><p>If <var>result</var> is null, <span data-x="concept-module-script-is-errored">is
errored</span>, or <span data-x="concept-module-script-has-instantiated">has instantiated</span>,
asynchronously complete this algorithm with <var>result</var>, and abort these steps.</p></li>
<li><p>Assert: <var>result</var>'s <span data-x="concept-script-record">record</span>'s
[[Status]] is "<code data-x="">uninstantiated</code>".</p></li>
<li><p>If <var>result</var> is null, asynchronously complete this algorithm with null, and abort
these steps.</p></li>
<li><p>If the <var>top-level module fetch</var> flag is set, <span data-x="fetch the descendants
of and instantiate a module script">fetch the descendants of and instantiate</span>
@@ -86871,9 +86844,8 @@ interface <dfn>NavigatorOnLine</dfn> {
success).</p>
<ol>
<li><p>If <var>module script</var> <span data-x="concept-module-script-is-errored">is
errored</span> or <span data-x="concept-module-script-has-instantiated">has instantiated</span>,
asynchronously complete this algorithm with <var>module script</var>, and abort these
<li><p>If <var>module script</var>'s <span data-x="concept-script-record">record</span> is null,
then asynchronously complete this algorithm with <var>module script</var> and abort these
steps.</p></li>
<li><p>Let <var>record</var> be <var>module script</var>'s <span
@@ -86927,39 +86899,13 @@ interface <dfn>NavigatorOnLine</dfn> {
<p>These invocations of the <span>internal module script graph fetching procedure</span> should
be performed in parallel to each other.</p>
<p>Wait for all invocations of the <span>internal module script graph fetching procedure</span>
to asynchronously complete, and let <var>results</var> be a <span>list</span> of the results,
corresponding to the same order they appeared in <var>urls</var>. Then, <span
data-x="list iterate">for each</span> <var>result</var> of <var>results</var>:</p>
<ol>
<li><p>If <var>result</var> is null, asynchronously complete this algorithm with null, aborting
these steps.</p></li>
<li><p>If <var>result</var> <span data-x="concept-module-script-is-errored">is errored</span>,
then <span data-x="concept-module-script-set-pre-instantiation-error">set the pre-instantiation
error</span> for <var>module script</var> to <var>result</var>'s <span
data-x="concept-module-script-error">error</span>. Asynchronously complete this algorithm with
<var>module script</var>, aborting these steps.</p></li>
</ol>
<div class="note">
<p>It is important to wait for all invocations to complete, and then iterate the results in
order, so that any fetching or pre-instantiation errors are deterministically surfaced to the
developer. Otherwise, a module script graph with multiple errors would surface a
nondeterministically chosen error, making debugging quite difficult.</p>
<p>As an unobservable optimization, implementations can quit early if an invocation returns
null or an <span data-x="concept-module-script-is-errored">errored</span> <span>module
script</span>, and all previous invocations have completed already. However, this is optimizing
for the rare failure case, so likely not worth the trouble.</p>
</div>
<p>If any of the invocations of the <span>internal module script graph fetching procedure</span>
asynchronously complete with null, asynchronously complete this algorithm with null, aborting
these steps.</p>
<p>If we've reached this point, all of the <span>internal module script graph fetching
procedure</span> invocations have asynchronously completed, with <span data-x="module
script">module scripts</span> that are not <span
data-x="concept-module-script-is-errored">errored</span>. Asynchronously complete this algorithm
with <var>module script</var>.</p>
<p>Otherwise, wait until all of the <span>internal module script graph fetching procedure</span>
invocations have asynchronously completed. Asynchronously complete this algorithm with
<var>module script</var>.</p>
</li>
</ol>
@@ -86979,30 +86925,92 @@ interface <dfn>NavigatorOnLine</dfn> {
completes with <var>result</var>.</p></li>
<li>
<p>If <var>result</var> is null or <span data-x="concept-module-script-is-errored">is
errored</span>, then asynchronously complete this algorithm with <var>result</var>.</p>
<p>If <var>result</var> is null, then asynchronously complete this algorithm with
<var>result</var>.</p>
<p class="note">In this case, there was an error fetching the descendants, or one of them
failed to parse, or was previously marked as errored. We will not attempt to instantiate.</p>
<p class="note">In this case, there was an error fetching one or more of the descendants. We
will not attempt to instantiate.</p>
</li>
<li><p>Let <var>record</var> be <var>result</var>'s <span
data-x="concept-script-record">record</span>.</p></li>
<li><p>Let <var>parse error</var> be the result of <span>finding the first parse error</span>
given <var>result</var>.</p></li>
<li>
<p>Perform <var>record</var>.<span data-x="js-Instantiate">Instantiate</span>().</p>
<li><p>If <var>parse error</var> is null, then:</p>
<p class="note">This step will recursively call <span data-x="js-Instantiate">Instantiate</span>
on all of the module's uninstantiated dependencies.</p>
<ol>
<li><p>Let <var>record</var> be <var>result</var>'s <span
data-x="concept-script-record">record</span>.</p></li>
<li>
<p>Perform <var>record</var>.<span data-x="js-Instantiate">Instantiate</span>().</p>
<p class="note">This step will recursively call <span
data-x="js-Instantiate">Instantiate</span> on all of the module's uninstantiated
dependencies.</p>
<p>If this throws an exception, ignore it for now; it is stored as <var>result</var>'s <span
data-x="concept-module-script-error">error</span>, and will be reported when we <span
data-x="run a module script">run</span> <var>result</var>.</p>
<p>If this throws an exception, set <var>result</var>'s <span
data-x="concept-script-error-to-rethrow">error to rethrow</span> to that exception.</p>
</li>
</ol>
</li>
<li><p>Otherwise, set <var>result</var>'s <span data-x="concept-script-error-to-rethrow">error to
rethrow</span> to <var>parse error</var>.</p></li>
<li><p>Asynchronously complete this algorithm with <var>result</var>.</p></li>
</ol>
<p>To <dfn data-x="finding the first parse error">find the first parse error</dfn> given a root
<var>moduleScript</var> and an optional <var>discoveredSet</var>:</p>
<ol>
<li><p>Let <var>moduleMap</var> be <var>moduleScript</var>'s <span>settings object</span>'s
<span data-x="concept-settings-object-module-map">module map</span>.</p></li>
<li><p>If <var>discoveredSet</var> was not given, let it be an empty <span>set</span>.</p></li>
<li><p><span data-x="list append">Append</span> <var>moduleScript</var> to
<var>discoveredSet</var>.</p></li>
<li><p>If <var>moduleScript</var>'s <span data-x="concept-script-record">record</span> is null,
then return <var>moduleScript</var>'s <span data-x="concept-script-parse-error">parse
error</span>.</p></li>
<li><p>Let <var>childSpecifiers</var> be the value of <var>moduleScript</var>'s <span
data-x="concept-script-record">record</span>'s [[RequestedModules]] internal slot.</p></li>
<li><p>Let <var>childURLs</var> be the <span>list</span> obtained by calling <span>resolve a
module specifier</span> once for each item of <var>childSpecifiers</var>,
given <var>moduleScript</var> and that item. (None of these will ever fail, as otherwise
<var>moduleScript</var> would have been marked as itself having a parse error.)</p></li>
<li><p>Let <var>childModules</var> be the <span>list</span> obtained by <span
data-x="map get">getting each value</span> in <var>moduleMap</var> whose key is given by an
item of <var>childURLs</var>.</p></li>
<li>
<p><span data-x="list iterate">For each</span> <var>childModule</var> of
<var>childModules</var>:</p>
<ol>
<li><p>Assert: <var>childModule</var> is a <span>module script</span> (i.e., it is not "<code
data-x="">fetching</code>" or null); by now all <span data-x="module script">module
scripts</span> in the graph rooted at <var>moduleScript</var> will have successfully been
fetched.</p></li>
<li><p>If <var>discoveredSet</var> already <span data-x="list contains">contains</span>
<var>childModule</var>, <span>continue</span>.</p></li>
<li><p>Let <var>childParseError</var> be the result of <span>finding the first parse
error</span> given <var>childModule</var> and <var>discoveredSet</var>.</p></li>
<li><p>If <var>childParseError</var> is not null, return <var>childParseError</var>.</p></li>
</ol>
</li>
<li><p>Return null.</p></li>
</ol>
<h5 id="creating-scripts">Creating scripts</h5>
<p>To <dfn data-x="creating a classic script">create a classic script</dfn>, given a
@@ -87024,6 +87032,9 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>Set <var>script</var>'s <span>muted errors</span> to <var>muted errors</var>.</p></li>
<li><p>Set <var>script</var>'s <span data-x="concept-script-parse-error">parse error</span> and
<span data-x="concept-script-error-to-rethrow">error to rethrow</span> to null.</p></li>
<li>
<p>Let <var>result</var> be <span data-x="js-ParseScript">ParseScript</span>(<var>source</var>,
<var>settings</var>'s <span data-x="environment settings object's Realm">Realm</span>,
@@ -87034,10 +87045,11 @@ interface <dfn>NavigatorOnLine</dfn> {
</li>
<li>
<p>If <var>result</var> is a <span>List</span> of errors, then:</p>
<p>If <var>result</var> is a <span>list</span> of errors, then:
<ol>
<li><p>Set <var>script</var>'s <span data-x="concept-script-parse-error">parse error</span> to
<li><p>Set <var>script</var>'s <span data-x="concept-script-parse-error">parse error</span> and
its <span data-x="concept-script-error-to-rethrow">error to rethrow</span> to
<var>result</var>[0].</p></li>
<li><p>Return <var>script</var>.</p></li>
@@ -87071,6 +87083,9 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>Set <var>script</var>'s <span>settings object</span> to <var>settings</var>.</p></li>
<li><p>Set <var>script</var>'s <span data-x="concept-script-parse-error">parse error</span> and
<span data-x="concept-script-error-to-rethrow">error to rethrow</span> to null.</p></li>
<li>
<p>Let <var>result</var> be <span data-x="js-ParseModule">ParseModule</span>(<var>source</var>,
<var>settings</var>'s <span data-x="environment settings object's Realm">Realm</span>,
@@ -87081,7 +87096,7 @@ interface <dfn>NavigatorOnLine</dfn> {
</li>
<li>
<p>If <var>result</var> is a <span>List</span> of errors, then:</p>
<p>If <var>result</var> is a <span>list</span> of errors, then:</p>
<ol>
<li><p>Set <var>script</var>'s <span data-x="concept-script-parse-error">parse error</span> to
@@ -87148,10 +87163,10 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>Let <var>evaluationStatus</var> be null.</p></li>
<li><p>If <var>script</var>'s <span data-x="concept-script-parse-error">parse error</span> is not
null, then set <var>evaluationStatus</var> to Completion { [[Type]]: throw, [[Value]]:
<var>script</var>'s <span data-x="concept-script-parse-error">parse error</span>, [[Target]]:
empty }.</p></li>
<li><p>If <var>script</var>'s <span data-x="concept-script-error-to-rethrow">error to
rethrow</span> is not null, then set <var>evaluationStatus</var> to Completion { [[Type]]: throw,
[[Value]]: <var>script</var>'s <span data-x="concept-script-error-to-rethrow">error to
rethrow</span>, [[Target]]: empty }.</p></li>
<li>
<p>Otherwise, set <var>evaluationStatus</var> to <span
@@ -87233,10 +87248,10 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>Let <var>evaluationStatus</var> be null.</p></li>
<li><p>If <var>script</var> <span data-x="concept-module-script-is-errored">is errored</span>,
then set <var>evaluationStatus</var> to Completion { [[Type]]: throw, [[Value]]:
<var>script</var>'s <span data-x="concept-module-script-error">error</span>, [[Target]]:
empty }.</p></li>
<li><p>If <var>script</var>'s <span data-x="concept-script-error-to-rethrow">error to
rethrow</span> is not null, then set <var>evaluationStatus</var> to Completion { [[Type]]: throw,
[[Value]]: <var>script</var>'s <span data-x="concept-script-error-to-rethrow">error to
rethrow</span>, [[Target]]: empty }.</p></li>
<li>
<p>Otherwise:</p>
@@ -88132,8 +88147,8 @@ import "https://example.com/foo/../module2.js";</pre>
<li><p>Assert: <var>resolved module script</var> is a <span>module script</span> (i.e., is not
null or "<code data-x="">fetching</code>").</p></li>
<li><p>Assert: <var>resolved module script</var> is not <span
data-x="concept-module-script-is-errored">errored</span>.</p>
<li><p>Assert: <var>resolved module script</var>'s <span
data-x="concept-script-record">record</span> is not null.</p>
<li><p>Return <var>resolved module script</var>'s <span
data-x="concept-script-record">record</span>.</p></li>

0 comments on commit 165101a

Please sign in to comment.