1759206 - Module is not bytecode encoded if the second request is evaluated first

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Tooru Fujisawa [:arai]]

from https://phabricator.services.mozilla.com/D140554

Bug 1436400 patch introduces JSBC support for module script, but the encoding requires the following 2 fields available:

• ScriptLoadRequest.mCacheInfo field
• JSScript reference in ModuleObject's ScriptSlot

ModuleObject's ScriptSlot is cleared on the evaluation, so the encode must be done before the evaluation.

ScriptLoadRequest.mCacheInfo field is populated only for the first ModuleLoadRequest that triggers network/cache request,
and the subsequent ModuleLoadRequest for the same URL waits for the ModuleScript and uses it,
where ScriptLoadRequest.mCacheInfo is not inherited from the first request, and there's no reference to the first request.

So, if the subsequent ModuleLoadRequest's module is evaluated first,
bytecode encoding cannot be performed because:

• The first evaluation lacks ScriptLoadRequest.mCacheInfo field
• The second evaluation lacks ModuleObject's ScriptSlot

The possible solution would be one of:

• Somehow keep the JSScript reference after evaluation
• Inherit or copy the ScriptLoadRequest.mCacheInfo from the first request to subsequent requests

Comment 1

[Tooru Fujisawa [:arai]]

If the ScriptSlot needs to be kept after the first evaluation due to top-level-await (see bug 1758322),
this bug can also be solved by not-clearing the slot.

Comment 2

[Tooru Fujisawa [:arai]]

The JSScript* reference is passed to JS::FinishIncrementalEncoding, but what the bytecode encoding needs is ScriptSource, not the top-level JSScript*.
Also, the ScriptSource should be kept by functions' scripts anyway.
So, we could expose the ScriptSource and add ScriptSource variant of JS::FinishIncrementalEncoding, in order to avoid keeping the top-level JSScript unnecessarily.

Comment 3

[Tooru Fujisawa [:arai]]

ModuleObject already has ScriptSourceObjectSlot that holds ScriptSourceObject, and the slot is never cleared.

So this can be fixed by:

• Add public API that returns ScriptSourceObject of given ModuleObject
• Add ScriptSourceObject variant of JS::FinishIncrementalEncoding
• Use ScriptSourceObject instead of JSScript in ScriptLoader

Comment 4

[Tooru Fujisawa [:arai]]

Apparently we don't expose ScriptSourceObject in any other place.
we might be better using ModuleObject itself in the public API instead.

Comment 5

[Andreas Farre [:farre]]

:arai, could you set a severity on this?

Comment 6

[Tooru Fujisawa [:arai]]

This has no impact.
Just that cache mechanism introduced by bug 1436400 isn't used in some edge case.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1759206 - Part 1: Add module variant of JS::FinishIncrementalEncoding. r?nbp!

Depends on D141742

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1759206 - Part 2: Use ModuleObject instead of script for encoding module bytecode cache. r?nbp!

Depends on D141743

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e76b6d814cc
Part 1: Add module variant of JS::FinishIncrementalEncoding. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d2d7b3b5fe70
Part 2: Use ModuleObject instead of script for encoding module bytecode cache. r=nbp

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0e76b6d814cc
https://hg.mozilla.org/mozilla-central/rev/d2d7b3b5fe70