Skip to content

Commit

Permalink
Fix module script error handling, again
Browse files Browse the repository at this point in the history 
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, the error fields of the script struct were
converted to a single error field, 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.
  • Loading branch information
@domenic
domenic committed Sep 6, 2017
1 parent dce999a commit 751ec3e
Showing 1 changed file with 119 additions and 107 deletions.
226 changes: 119 additions & 107 deletions source
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86253,11 +86253,20 @@ interface <dfn>NavigatorOnLine</dfn> {
script">module scripts</span>; or null. In the former two cases, it represents a parsed script;
null represents a failure parsing.</p></dd>

<dt>A <dfn data-dfn-for="script" data-export="" data-x="concept-script-parse-error">parse
error</dfn></dt>
<dt>An <dfn data-dfn-for="script" data-export="" data-x="concept-script-error">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>
<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">There are two main categories of such errors: errors parsing the script's source
text, in which case the script's <span data-x="concept-script-record">record</span> will
be null; or more general errors in dependencies of a <span>module script</span>. This
distinction is used by the algorithm for <span>finding the first parse error</span>.</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>
</dl>


Expand Down Expand Up @@ -86316,41 +86325,6 @@ interface <dfn>NavigatorOnLine</dfn> {

</dl>

<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
Expand Down Expand Up @@ -86781,13 +86755,6 @@ 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 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>
<var>result</var> given <var>destination</var> and <var>visited set</var>. Otherwise,
Expand Down Expand Up @@ -86898,9 +86865,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
Expand Down Expand Up @@ -86954,39 +86920,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>

Expand All @@ -87006,30 +86946,100 @@ 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>If <var>parse error</var> is not null, then set <var>result</var>'s <span
data-x="concept-script-error">error</span> to <var>parse error</var>.</p></li>

<li>
<p>Perform <var>record</var>.<span data-x="js-Instantiate">Instantiate</span>().</p>
<p>Otherwise:</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 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">error</span> to that exception.</p>
</li>
</ol>
</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-error">error</span>.</p>

<p class="note">It's important that we check if <var>moduleScript</var>'s <span
data-x="concept-script-record">record</span> is null, and <em>not</em> simply check that
<var>moduleScript</var>'s <span data-x="concept-script-error">error</span> is non-null. This
ensures we only catch errors from parsing <var>moduleScript</var>, and not instantiation errors
or errors from parsing its dependencies that were stored in the <span
data-x="concept-script-error">error</span> field for later re-throwing.</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
Expand Down Expand Up @@ -87060,10 +87070,10 @@ interface <dfn>NavigatorOnLine</dfn> {
</li>

<li><p>If <var>result</var> is a <span>List</span> of errors, then set <var>script</var>'s <span
data-x="concept-script-parse-error">parse error</span> to <var>result</var>[0].</p></li>
data-x="concept-script-error">error</span> to <var>result</var>[0].</p></li>

<li><p>Otherwise, set <var>script</var>'s <span data-x="concept-script-record">record</span> to
<var>result</var>.</p></li>
<var>result</var> and its <span data-x="concept-script-error">error</span> to null.</p></li>

<li><p>Return <var>script</var>.</p></li>
</ol>
Expand Down Expand Up @@ -87099,7 +87109,7 @@ interface <dfn>NavigatorOnLine</dfn> {
<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
<li><p>Set <var>script</var>'s <span data-x="concept-script-error">error</span> to
<var>result</var>[0].</p></li>

<li><p>Return <var>script</var>.</p></li>
Expand All @@ -87120,8 +87130,8 @@ interface <dfn>NavigatorOnLine</dfn> {
<ol>
<li><p>Let <var>error</var> be a new <code>TypeError</code> exception.</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>
<li><p>Set <var>script</var>'s <span data-x="concept-script-error">error</span> to
<var>error</var>.</p></li>

<li><p>Return <var>script</var>.</p></li>
</ol>
Expand All @@ -87137,6 +87147,8 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>Set <var>script</var>'s <span data-x="concept-script-record">record</span> to
<var>result</var>.</p></li>

<li><p>Set <var>script</var>'s <span data-x="concept-script-error">error</span> to null.</p></li>

<li><p>Set <var>script</var>'s <span data-x="concept-module-script-base-url">base URL</span> to
<var>baseURL</var>.</p></li>

Expand Down Expand Up @@ -87169,10 +87181,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
<li><p>If <var>script</var>'s <span data-x="concept-script-error">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>
<var>script</var>'s <span data-x="concept-script-error">error</span>, [[Target]]: empty
}.</p></li>

<li>
<p>Otherwise, set <var>evaluationStatus</var> to <span
Expand Down Expand Up @@ -87252,10 +87264,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">error</span> is not
null, then set <var>evaluationStatus</var> to Completion { [[Type]]: throw, [[Value]]:
<var>script</var>'s <span data-x="concept-script-error">error</span>, [[Target]]: empty
}.</p></li>

<li>
<p>Otherwise:</p>
Expand Down Expand Up @@ -88140,8 +88152,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>
Expand Down

0 comments on commit 751ec3e

Please sign in to comment.