1941472 - Stop setting JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars

Status: RESOLVED FIXED

(Core :: XPConnect, task, P3)

Description

[Tooru Fujisawa [:arai]]

Split out from bug 1881888.

deoptimizeModuleGlobalVars was added for backward compatibility layer in the system module loader, to keep the global variable behavior match to the JSM.
Given the JSM loader and the JSM to ESM shim are going to be removed, this option is no longer necessary, but this adds another behavior difference due to XPCOM and some privileged API that allows getting strong ref from weak ref,
given that in the standard ECMAScript modules, non-exported module globals no longer have the same lifetime as the module itself.
Some system module code expects the globals to have the same lifetime as the module itself, and they use weak reference to those values to simplify the cleanup (e.g. addObserver without removeObserver).

Currently the following usage looks affected:

• nsIObserverService::addObserver with ownsWeak=true, and wrappers around it
• any other things takes nsISupportsWeakReference
• ChromeUtils.nondeterministicGetWeakMapKeys

The problematic behaviors are the following:

• get strong ref from weak ref
• iterate over weak-ref-ed objects

Given the above, flipping the option can break some code (e.g. bug 1940741 ), and this need to be done carefully.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Bug 1941472 - Stop setting JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars. r?jonco!

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90f5357eb239
Stop setting JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars. r=jonco

Comment 3

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/90f5357eb239

Comment 4

[Serban Stanca [:SerbanS]]

Backed out as requested by arai for causing Bug 1946592.

• Backout link

Comment 5

[Reem Hamoui]

Attachment: WIP: Backed out 2 changesets (Bug 1941472, bug 1938859) as requested by arai for causing Bug 1946592. a=backout

Backed out changeset e9c90fbcc514 (bug 1938859)
Backed out changeset 90f5357eb239 (bug 1941472)

Comment 6

[Reem Hamoui]

Hey all, not sure what happened here ^?
I was pushing up another patch I was working on, and this WIP patch also got pushed from my end, and it shows me as the author?

Is it safe for me to just close it?

Comment 7

[Reem Hamoui]

Spoke to :dholbert and he gave me the green-light to abandon the patch.
It seems the bug was re-opened a couple of hours before i accidentally pushed a patch, so I'll leave it open

Comment 8

[Tooru Fujisawa [:arai]]

There was misunderstanding about the effect of deoptimizeModuleGlobalVars.
Module's varScope contains all top-level variables, including lexial variables, such as let, const, and class,
and iterating over modulepc.varScope().bindings(pc_) results in handling all of them.

We should audit all top-level variables, not only vars.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/352cc7cae254
Stop setting JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars. r=jonco

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/352cc7cae254

Comment 11

[Atila Butkovits]

Backed out for causing high frequency failures at browser_bannerClicking.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/6900871c5dfcdeb2e0de821f57f877b64d43bd5e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=9a3a01f76cb54396262610ed1487e7cd994a2e61&selectedTaskRun=eS5P6QbUTlqiztIrMXgReQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=497828384&repo=autoland&lineNumber=5608

Comment 12

[Cosmin Sabou [:CosminS]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/6900871c5dfc

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f1ca8a80220
Stop setting JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars. r=jonco

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/4f1ca8a80220