1461751 - Simplify module resolve hook

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco) (PTO until 3rd April)]

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.

Comment 1

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: bug1461751-simplify-module-resolve-hook

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.

Comment 2

[Luke Wagner [:luke]]

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...

Comment 3

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(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.

Comment 4

[Jon Coppeard (:jonco) (PTO until 3rd April)]

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

Requesting additional review for script loader changes.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e862899dca3f
Simplify module resolve hook to be a function pointer r=luke r=baku

Comment 6

[Stefan Hindli [:stefan_hindli]]

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