Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Description • 8 years ago

The HTML spec was updated to eagerly instantiate JS modules as they are loaded:

https://github.com/whatwg/html/pull/1615

The problem this is addressing is handling instantiation errors in an imported module that may be a dependency of multiple top-level modules (such dependencies are shared).

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 1 • 8 years ago

The patch adds instantiation state to a module script and attempts to instantiate modules after their dependencies have been loaded.  Any error is recorded in the module script.  Subsequent uses of a failed module (e.g. import via a different top-level module) will throw the same error rather than using the failed module.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 8 years ago

Comment on attachment 8782003 [details] [diff] [review]
bug1295978-module-eager-instantiation

bkelly said he could take a look at this in couple of days.

jonco, is it documented somewhere how module loading works? Like in the bug adding support for module loading or something.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 3 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #2)
We're following the HTML spec pretty closely, but apart from that no.

Would you like me to add a comment explaining how all this works?  I'm not sure what the best place is for this kind of documentation.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 8 years ago

Somewhere in ScriptLoader perhaps?

	Ben Kelly [:bkelly, not reviewing]
	Comment 5 • 8 years ago

FYI, I plan to review this tomorrow.  I want to make sure I understand the previous patch that landed before reviewing this one.  Sorry for the delay.

	Ben Kelly [:bkelly, not reviewing]
	Comment 6 • 8 years ago

Comment on attachment 8782003 [details] [diff] [review]
bug1295978-module-eager-instantiation

Review of attachment 8782003 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good, but I think perhaps we're leaving the nsModuleScript in an inconsistent state if we early exit from InstantiateModuleTree().

::: dom/base/nsScriptLoader.cpp
@@ +1112,5 @@
>  
> +bool
> +nsScriptLoader::InstantiateModuleTree(nsModuleLoadRequest* aRequest)
> +{
> +  // Perform eager instantiation of the loaded module tree.

MOZ_ASSERT(aRequest)

@@ +1115,5 @@
> +{
> +  // Perform eager instantiation of the loaded module tree.
> +
> +  nsModuleScript* ms = aRequest->mModuleScript;
> +  if (!ms->ModuleRecord()) {

Please MOZ_ASSERT(ms) or check it in the if-statement.  If you de-referenced mModuleScript itself you would get the assert for free since its a RefPtr.  Since you are using a local bare pointer, though, you need to check.

@@ +1125,5 @@
> +    return false;
> +  }
> +
> +  nsresult rv = EnsureModuleResolveHook(jsapi.cx());
> +  NS_ENSURE_SUCCESS(rv, false);

Should we mark the module as Errored if we fail early due to an internal issue or OOM?  It does not look like there is an opportunity to re-instantiate a module later.  Is there?

@@ +1139,5 @@
> +    }
> +    MOZ_ASSERT(!exception.isUndefined());
> +  }
> +
> +  // Recursively mark this module and any uninstantiated dependencies as

nit: Maybe change this to note you are doing a depth-first search.  And maybe remove "recursive" since its not really doing any recursive calls.

@@ +2131,5 @@
>        nsModuleLoadRequest* request = aRequest->AsModuleRequest();
>        MOZ_ASSERT(request->mModuleScript);
>        MOZ_ASSERT(!request->mOffThreadToken);
> +      nsModuleScript* ms = request->mModuleScript;
> +      MOZ_ASSERT(!ms->IsUninstantiated());

Again, this suggests we should not leave the module in the Uninstantiated state if we early error in InstantiateModuleTree().

Or we could try to trigger the instantiation again at this point if we believe this will mainly happen for transient errors like OOM conditions.

	Ben Kelly [:bkelly, not reviewing]
	Comment 7 • 8 years ago

Comment on attachment 8782003 [details] [diff] [review]
bug1295978-module-eager-instantiation

Review of attachment 8782003 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I think I see how the early exit is handled now.  It triggers a LoadFailed().  That means we should never enter EvaluateScript() where we assert !Uninstatiated.  Sorry for my confusion.

r=me with other nits addressed.  Thanks!

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 8 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #7)

Yes, InstantiateModuleTree must always succeed before EvaluateScript happens.

Thanks for the review!  Nits have been fixed.

	Pulsebot
	Comment 9 • 8 years ago

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5048d88722
Eagerly instantiate module dependencies r=bkelly

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ae5048d88722