1666683 - Check input CompileOptions when decoding, or getting from cache

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Tooru Fujisawa [:arai]]

same thing as https://searchfox.org/mozilla-central/rev/35d927df97900a57ecb562ad13909e392440b0fb/js/src/vm/JSScript.cpp#1036-1054

Comment 1

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/rev/35d927df97900a57ecb562ad13909e392440b0fb/js/src/vm/JSScript.cpp#987-994 will be checked by bug 1662274

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Abort XDR if CompileOptions differ. r?tcampbell!

Depends on D90585

Comment 3

[Ted Campbell [:tcampbell]]

Probably want all the "input" flags listed here https://searchfox.org/mozilla-central/rev/f21850ca45036ddb84cd25aa355a6969d7c94c4f/js/src/frontend/SharedContext.cpp#56-67

Comment 4

[Tooru Fujisawa [:arai]]

In ScriptLoader, same CompileOption is passed to both JS::Compile and JS::DecodeScriptMaybeStencil.
but in other places, callers aren't passing same CompileOption as JS::Compile to decoding API.

1. JS::DecodeScript doesn't receive CompileOptions.
2. JS::DecodeMultiOffThreadScripts receives CompileOptions, but the option is shared across multiple scripts that can be compiled with different options

and in remaining consumers, the decoded script is stored into cache and used later, and compilation happens when the cache isn't found.
It means we should check CompileOptions vs ImmutableScriptFlags when getting from the cache, instead of when decoding.

I'll add an API that checks CompileOptions vs ImmutableScriptFlags, for existing consumers.

once we replace the cache to store stencil, we can compare CompileOptions directly between cache and compilation input.

Comment 5

[Tooru Fujisawa [:arai]]

plan:

1. for JS::DecodeScript, add CompileOptions parameter and modify the consumer to pass the same options as JS::Compile, and check the options vs flags internally
2. for JS::DecodeMultiOffThreadScripts, modify the consumers to pass options that is consistent as much as possible (but still not all options can be same), and check some options vs flags internally
3. for JS::DecodeMultiOffThreadScripts consumers, when getting a decoded script from the cache, make it receive CompileOptions and check options vs flags

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 1: Reorder CompileOptions parameter. r?tcampbell!

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 2: Add CompileOptions parameter to JS::DecodeScript, as a placeholder. r?tcampbell!

This adds the parameter, without using it, also without passing the correct
value.
The later patch will change the consumer of those API to pass the correct value,
and then use the passed parameter.

Depends on D92403

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 3: Add options parameter to ScriptPreloader::GetCachedScript. r?tcampbell!

Depends on D92404

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 4: Add options parameter to ReadCachedScript in mozJSLoaderUtils. r?tcampbell!

Depends on D92405

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 5: Add options parameter to nsXPConnect::ReadScript. r?tcampbell!

Depends on D92406

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 6: Add ScriptPreloader::FillCompileOptionsForCachedScript. r?tcampbell!

Depends on D92407

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 7: Fix shell offThreadDecodeScript to use the same option as execute. r?tcampbell!

Depends on D92408

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 8: Move options_ field from XDROffThreadDecoder to XDRDecoder. r?tcampbell!

Depends on D92409

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 9: Add XDRState::isMultiDecode as a preparation to check options conditionally. r?tcampbell!

Depends on D92410

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 10: Check CompileOptions when decoding XDR. r?tcampbell!

Depends on D92411

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 11: Add FillImmutableFlagsFromCompileOptionsForTopLevel to centralize CompileOptions-ImmutableScriptFlags interaction. r?tcampbell!

Depends on D92412

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1666683 - Part 12: Add JS::CheckCompileOptionsMatch and check options in ScriptPreloader. r?tcampbell!

Depends on D92413

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5f2032ed919
Part 1: Reorder CompileOptions parameter. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/a6da1179a589
Part 2: Add CompileOptions parameter to JS::DecodeScript, as a placeholder. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/777c94a3c977
Part 3: Add options parameter to ScriptPreloader::GetCachedScript. r=tcampbell,kmag
https://hg.mozilla.org/integration/autoland/rev/3014569a17c7
Part 4: Add options parameter to ReadCachedScript in mozJSLoaderUtils. r=tcampbell,kmag
https://hg.mozilla.org/integration/autoland/rev/8ecde54fab31
Part 5: Add options parameter to nsXPConnect::ReadScript. r=tcampbell,kmag
https://hg.mozilla.org/integration/autoland/rev/483e81a2dbd7
Part 6: Add ScriptPreloader::FillCompileOptionsForCachedScript. r=tcampbell,kmag
https://hg.mozilla.org/integration/autoland/rev/3e4b7122f576
Part 7: Fix shell offThreadDecodeScript to use the same option as execute. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/50cd482cdb64
Part 8: Move options_ field from XDROffThreadDecoder to XDRDecoder. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/5a6df8bb1ab5
Part 9: Add XDRState::isMultiDecode as a preparation to check options conditionally. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/240b2e7d6afc
Part 10: Check CompileOptions when decoding XDR. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/2fbbdd37bb54
Part 11: Add FillImmutableFlagsFromCompileOptionsForTopLevel to centralize CompileOptions-ImmutableScriptFlags interaction. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/8827caa78683
Part 12: Add JS::CheckCompileOptionsMatch and check options in ScriptPreloader. r=tcampbell,kmag

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c5f2032ed919
https://hg.mozilla.org/mozilla-central/rev/a6da1179a589
https://hg.mozilla.org/mozilla-central/rev/777c94a3c977
https://hg.mozilla.org/mozilla-central/rev/3014569a17c7
https://hg.mozilla.org/mozilla-central/rev/8ecde54fab31
https://hg.mozilla.org/mozilla-central/rev/483e81a2dbd7
https://hg.mozilla.org/mozilla-central/rev/3e4b7122f576
https://hg.mozilla.org/mozilla-central/rev/50cd482cdb64
https://hg.mozilla.org/mozilla-central/rev/5a6df8bb1ab5
https://hg.mozilla.org/mozilla-central/rev/240b2e7d6afc
https://hg.mozilla.org/mozilla-central/rev/2fbbdd37bb54
https://hg.mozilla.org/mozilla-central/rev/8827caa78683