ModuleObject entrains the top level script

RESOLVED FIXED in Firefox 64



9 months ago
7 months ago


(Reporter: jonco, Assigned: jonco)


(Blocks 1 bug)

61 Branch
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)



(2 attachments)



9 months ago
The top level JSScript for a module is only needed the first time the module is evaluated, but is kept alive by the ModuleObject.

Comment 1

7 months ago
Annoyingly the shell import.meta hook currently uses the module's script to get its filename to use for the 'url' property, and to fix that I'm going to move the hook's implementation to self-hosted JS.  But that doesn't work at the moment because we can currently call this hook while compiling to pre-create the metadata object if it doesn't already exist.

This patch makes us generate code to create/get the metadata object at runtime just like happens in the interpreter.
Assignee: nobody → jcoppeard
Attachment #9015588 - Flags: review?(jdemooij)

Comment 2

7 months ago
Patch to split ModuleObject::script() in to script() and maybeScript() and move implementation of the shell import.meta hook to self-hosted JS so we can get the module path easily.  Mostly the latter.

The ModuleObjet's script is set to null after it has been executed so we don't hold onto it.

A bug in the module loader could probably cause us to try and get the script for a module who's script is null so I made the check a release assert.
Attachment #9015590 - Flags: review?(sphink)
Comment on attachment 9015590 [details] [diff] [review]

Review of attachment 9015590 [details] [diff] [review]:

::: js/src/builtin/ModuleObject.cpp
@@ +1163,5 @@
>      RootedScript script(cx, self->script());
> +
> +    // The top-level script if a module is only ever executed once. Clear the
> +    // reference to prevent us keeping this alive unnecessarily.
> +    self->setReservedSlot(ScriptSlot, UndefinedValue());

I doubt it matters enough to risk messing with, but it seems like we'll know that this is unreachable after this, so the pre-barrier might end up keeping it alive for one more cycle than is strictly necessary.

::: js/src/shell/js.cpp
@@ +8008,5 @@
> +    JS_FN_HELP("setModuleMetadataHook", SetModuleMetadataHook, 1, 0,
> +"setModuleMetadataHook(function(module) {})",
> +"  Set the HostPopulateImportMeta hook to |function|.\n"
> +"  This hook is used to create the metadata object returned by import.meta for\n"
> +"  a module.  It should be implemented by the module loader."),

I'll just trust you that this is the right extension point for testing. (I mean, it makes sense, but I don't know the spec.)
Attachment #9015590 - Flags: review?(sphink) → review+

Comment 4

7 months ago
Comment on attachment 9015588 [details] [diff] [review]

Moving review since Jan is on PTO.
Attachment #9015588 - Flags: review?(jdemooij) → review?(nicolas.b.pierron)
Comment on attachment 9015588 [details] [diff] [review]

Review of attachment 9015588 [details] [diff] [review]:

This patch looks good to me except that we need to record the CompilerObject of MModuleMetadata.
r=me with one of the 2 alternatives:

::: js/src/jit/MIR.h
@@ +7792,5 @@
>  };
> +class MModuleMetadata : public MNullaryInstruction
> +{
> +    CompilerObject module_;

You need to either override `appendRoots` [1], or use a Constant to hold this object value like done with many `templateObject` [2] which are made to be resumed with recover instructions.

Attachment #9015588 - Flags: review?(nicolas.b.pierron) → review+

Comment 6

7 months ago
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
Ah thanks, I'll fix that.

Comment 7

7 months ago
Pushed by
Don't pre-create module metadata object when compiling r=nbp
Stop modules from entraining the top-level JSScript r=sfink

Comment 8

7 months ago
Pushed by
Don't pre-create module metadata object when compiling: remove trailing space a=eslint-fix

Comment 9

7 months ago
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1498980
You need to log in before you can comment on or make changes to this bug.