Closed Bug 1481196 Opened 2 years ago Closed 2 years ago

Make the result of compiling a module a JSScript

Categories

(Core :: JavaScript Engine, enhancement)

61 Branch
enhancement
Not set

Tracking

()

RESOLVED INVALID
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(3 files)

At the moment we have the situation where our APIs which compile modules return ModuleObjects.

Dynamic module import can happen from both classic scripts and module scripts so this will require treating modules and classic scripts interchangeably in a few places.  We should change these APIs to return JSScripts instead.

It also makes sense to make these APIs more similar to their classic script counterparts.
Change module APIs to use JSScript rather than ModuleObject.

I added JS::GetScriptRealm() and a version of Scriptability::Get() that takes a JSScript pointer.

I added JS::GetScriptGlobal() so we can initialise an AutoJSAPI in the browser as this currently uses the module object.
Attachment #8997932 - Flags: review?(jdemooij)
Script loader changes to refer to modules by a JSScript rather than a JSObject.
Attachment #8997934 - Flags: review?(amarchesini)
Comment on attachment 8997932 [details] [diff] [review]
bug1481196-compile-module-to-script-1

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

Nice, I like the consistency.

::: js/src/jsapi.h
@@ +3900,5 @@
>  SetModuleMetadataHook(JSRuntime* rt, ModuleMetadataHook func);
>  
>  /**
>   * Parse the given source buffer as a module in the scope of the current global
>   * of cx and return a source text module record.

Nit: update this comment? Also a few times below.
Attachment #8997932 - Flags: review?(jdemooij) → review+
Attachment #8997934 - Flags: review?(amarchesini) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba122021b8b5
Compile module scripts to a JSScript like we do for classic scripts r=jandem r=baku
https://hg.mozilla.org/mozilla-central/rev/ba122021b8b5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Unfortunately this approach is not going to work.  Referring to a module by its toplevel JSScript keeps that script alive indefinitely even though in most cases this will be garbage after the module has been loaded.  The same applies to classic scripts.  This can potentially keep around a lot of garbage.  We need a way to refer to both kinds of script that doesn't keep their toplevel JSScript alive.

I'm going to back out the patch in this bug, as well as bug 1482153 and bug 1485031 which depend on these changes.
Just running this past you for cursory review as it's not a straight backout due to intervening changes.
Attachment #9014066 - Flags: review?(jdemooij)
Attachment #9014066 - Flags: review?(jdemooij) → review+
Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.