Closed Bug 1461751 Opened Last year Closed Last year

Simplify module resolve hook

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

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.
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.
Attachment #8975886 - Flags: review?(luke)
Blocks: 1427610
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...
Attachment #8975886 - Flags: review?(luke) → review+
(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 on attachment 8975886 [details] [diff] [review]
bug1461751-simplify-module-resolve-hook

Requesting additional review for script loader changes.
Attachment #8975886 - Flags: review?(amarchesini)
Attachment #8975886 - Flags: review?(amarchesini) → review+
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
https://hg.mozilla.org/mozilla-central/rev/e862899dca3f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1462326
You need to log in before you can comment on or make changes to this bug.