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

Currently the module resolve hook (HostResolveImportedModule) is supplied as a JSFunction which is set on the global.  The browser wants to implement this in C++ so it would be simpler to have this as a standard C++ function pointer like we do for all the other embedding hooks we have.  It can also be set on the runtime since it doesn't vary between globals.

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

Patch to turn the module resolve hook into a function pointer.  This moves some complexity from the browser into the shell (which does have to call a JSFunction) which seems like the right thing to do.

	Luke Wagner [:luke]
	Comment 2 • 6 years ago

Comment on attachment 8975886 [details] [diff] [review]
bug1461751-simplify-module-resolve-hook

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

That does seem like the right trade off.  Feel free to ignore suggestion if there is clear local precedent against.

::: js/src/jsapi.h
@@ +4142,5 @@
>  extern JS_PUBLIC_API(bool)
>  Evaluate(JSContext* cx, const ReadOnlyCompileOptions& options,
>           const char* filename, JS::MutableHandleValue rval);
>  
> +using ModuleResolveHook = JSObject* (*)(JSContext*, HandleObject, HandleString);

Maybe MutableHandleObject instead of JSObject* return value?  In the limit, the callee could have an RAII object on the stack that causes a GC that makes returning a JSObject* fundamentally unsafe...

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

(In reply to Luke Wagner [:luke] from comment #2)
> Maybe MutableHandleObject instead of JSObject* return value?  In the limit,
> the callee could have an RAII object on the stack that causes a GC that
> makes returning a JSObject* fundamentally unsafe...

We do this in a lot of places, so I'm going to leave it.  I'm pretty sure the rooting analysis would catch the problem you describe, but I'll confirm that with Steve.

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

Comment on attachment 8975886 [details] [diff] [review]
bug1461751-simplify-module-resolve-hook

Requesting additional review for script loader changes.

	Pulsebot
	Comment 5 • 6 years ago

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e862899dca3f
Simplify module resolve hook to be a function pointer r=luke r=baku

	Stefan Hindli [:stefan_hindli]
	Comment 6 • 6 years ago
bugherder

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