Closed Bug 1461751 Opened 7 years ago Closed 7 years ago

Simplify module resolve hook

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

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)
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Regressions: 1463371
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: