Module records should be be discarded on link failure

[jonco3]

In https://html.spec.whatwg.org/#calling-scripts the spec currently says:

Let instantiationStatus be record.ModuleDeclarationInstantiation().
If instantiationStatus is an abrupt completion, report the exception given by instantiationStatus.[[Value]] for s and abort these steps.

If the ModuleDeclarationInstantiation fails for a record (or for one of its descendants because this happens recursively) then that module script will be left in the module map and can be used to satisfy subsequent requests to fetch a module script. ModuleDeclarationInstantiation will not attempt to re-instantiate a module record if called again for the same record, and incorrect execution will follow.

It seems the expectation is that the entire module tree will be discarded if ModuleDeclarationInstantation fails.

[domenic]

Hmm. I vaguely remember this being intentional, but I am not sure. Can you help me work through the consequences of leaving it in the map?

I thought the idea was that if a module fails to instantiate, then we should show the resulting error once, and further attempts to use it (e.g. by importing anything from it) will fail, since it has no exports. I think that is what is currently specified, and it seems to work OK.

You seem to be advocating for a different semantic, where we remove it from the map, and thus allow it to be re-fetched? That seems kind of dubious, since we won't get a chance to re-fetch unless someone later does dynamic module loading, and in the meantime we're just going to cause HostResolveImportedModule to fail repeatedly. And re-fetching seems unlikely to fix the problem.

[jonco3]

further attempts to use it (e.g. by importing anything from it) will fail, since it has no exports

The trouble is that's not what ES specifies for the behaviour of ModuleDeclarationInstantiation. This does not handle being called again after it fails the first time and as specced will complete successfully the second time without actually doing anything, leaving the module environment partially initialised.

I asked about this on es-discuss and the answer was that module loaders are expected to discard module records if there is an error linking.

If we remove module scripts from the map then as you say we will fail in HostResolveImportedModule or attempt to re-fetch. The latter does seem undesirable and perhaps we should record the fact that we failed and generate an appropriate error if the same module requested again at a later time.

[domenic]

Hmm, I see. I'm re-reading the es-discuss thread now. I guess we can try to to deal with this ourselves. I still can't see what the problems are with letting ModuleDeclarationInstantiation complete successfully the second time, but I guess you found some in fuzz testing?

[domenic]

If you have any ideas on how you are going to implement this, or how it could be specced, I'd appreciate them. Right now it looks quite hard to do with the current spec setup. In particular there's no real easy way to find out which entries in the module map need to get nulled out.

I guess the closest I have is looping through the tree of [[RequestedModules]], using "resolve a module specifier" to get to the module map key, and then nulling it out from there after all the relevant keys are collected?

[jonco3]

Not sure if this is what you meant, but the problem with letting
ModuleDeclarationInstantiation complete successfully the second time is
that it leaves the environment partially initialised. In our
implementation this caused a crash, and even if this were made to work you
would get totally unrelated errors when the module was executed (e.g.
reference error accessing a binding that should be present).

I agree that this looks hard to do with the current spec.
ModuleDeclarationInstantiation happens recursively over a module's imports
and doesn't give an indication of which one failed. Removing the whole
module graph could remove successfully loaded modules that are already in
use by a previously loaded module, which could result in a different copy
of them being reloaded at a later time.

I'll think some more about this.

On 13 July 2016 at 17:51, Domenic Denicola notifications@github.com wrote:

If you have any ideas on how you are going to implement this, or how it
could be specced, I'd appreciate them. Right now it looks quite hard to do
with the current spec setup. In particular there's no real easy way to find
out which entries in the module map need to get nulled out.

I guess the closest I have is looping through the tree of
[[RequestedModules]], using "resolve a module specifier" to get to the
module map key, and then nulling it out from there after all the relevant
keys are collected?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1545 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA0j0ogvksd00ETuShwEtuEewEtCdGIMks5qVReZgaJpZM4JLigQ
.

[domenic]

In our implementation this caused a crash, and even if this were made to work you would get totally unrelated errors when the module was executed (e.g. reference error accessing a binding that should be present).

Why would you call this an unrelated error? This seems like the correct response to me: you try to import from a module which has previously failed to load, and you get a ReferenceError using that binding.

[jonco3]

It's not necessarily that binding though, it's whatever the first uninitialised binding is that happens to be accessed when the previously-failed module is executed.

[domenic]

Ah, I think I see. Meanwhile I guess the es-discuss thread has generated some interesting ideas...

[domenic]

@jonco3 do you have a good understanding of the conclusion of the es-discuss thread, and how to possibly integrate it into the spec's model?

[jonco3]

@domenic Yes, I'm working on this at the moment.

[jonco3]

OK, I think this is what we need to do:

Add the following state to a module script:

• boolean flag 'is instantiated', initially false
• JS exception value 'instantiation error', initially undefined

At the end of internal module script tree fetching procedure, when fetching the descendants of a module script asynchronously completes add an additional step 'instantiate fetched module script tree'

This procedure would do something like this:

• call ModuleDeclarationInstantiation for result module script
• then recursively traverse all dependencies of module script for which 'is instantiated' is false, updating them as follows:
  • if instantiation failed, set 'instantiation error' to the error object and set the module script's module record to null
  • set 'is instantiated' to true

Update HostResolveImportedModule to check the whether the module script it finds has 'instantiation error' set and if so re-throw it.

Remove the call to ModuleDeclarationInstantiation from 'run a module script' and instead check whether 'instantiation error' is set and if so re-throw it.

Oh, I guess this also means we need to track the dependencies of a module script which are not recorded at the moment. I think implementations would need to do that anyway even though it's not explicit in the spec at the moment.

How does this sound to you? It's more complicated than I'd like but I don't see a way around that.

[domenic]

This sounds pretty doable. Thanks so much for going to the trouble of figuring it out. A few minor questions.

First, I guess I just want to make sure you think it's correct to move ModuleDeclarationInstantiation earlier. It really feels like something that should be done at "run a module script" time, instead of "fetch a module script tree" time. But I think you make it work, by storing any errors and re-throwing then during run-time. So I think it works.

if instantiation failed, set 'instantiation error' to the error object and set the module script's module record to null

Which error object? The one from "call ModuleDeclarationInstantiation for result module script"? That will be the one from the first failed dependency, right?

This step means that if a depends on b and c, and instantiating b fails, then c will get marked as bad. Future module script trees will not be able to depend on it without also getting marked as bad. Is that OK?

Also, why set the module record to null? We'd need to start adding guards to prevent executing null module records, right? But aren't those guards already covered by the guard against "instantiation error"?

Update HostResolveImportedModule to check the whether the module script it finds has 'instantiation error' set and if so re-throw it.

Since the error can be itself undefined, I guess we need another boolean, right? Maybe a tri-state "uninstantiated", "errored", "instantiated"?

Oh, I guess this also means we need to track the dependencies of a module script which are not recorded at the moment. I think implementations would need to do that anyway even though it's not explicit in the spec at the moment.

I think this is reachable via the module script's module record's [[RequestedModules]], each being passed to "resolve a module specifier" and then the result looked up in the module map. A bit indirect, but it works :).

[jonco3]

First, I guess I just want to make sure you think it's correct to move ModuleDeclarationInstantiation earlier. It really feels like something that should be done at "run a module script" time

I think it's fine. The ES spec says it's OK to instantiate modules ahead of time as long as error reporting is deferred until TopLevelModuleEvaluationJob runs (not that we're actually doing TopLevelModuleEvaluationJob exactly but something hopefully equivalent).

Which error object? The one from "call ModuleDeclarationInstantiation for result module script"?

Yes that one.

This step means that if a depends on b and c, and instantiating b fails, then c will get marked as bad.

No, that shouldn't happen. b and c will be instantiated separately at the end of the respective calls to internal module script tree fetching procedure for those modules. The situation in multiple modules will get marked as bad is if there is a cyclic dependency. Then all modules in the cycle will be marked as bad, but this is correct.

Also, why set the module record to null?

According to ES we must discard the module record if instantiation fails so it seems prudent to remove any reference to it.

Since the error can be itself undefined

Oh I guess it can, that's annoying. In that case we probably need a tri-state as you suggested.

I think this is reachable via the module script's module record's

Yes, that sounds like it would work.

[domenic]

This step means that if a depends on b and c, and instantiating b fails, then c will get marked as bad.

No, that shouldn't happen. b and c will be instantiated separately at the end of the respective calls to internal module script tree fetching procedure for those modules. The situation in multiple modules will get marked as bad is if there is a cyclic dependency. Then all modules in the cycle will be marked as bad, but this is correct.

Ah, I see, after working out an example in detail this makes sense. Perfect.

I'll start speccing this. Thanks so much for your help!!