1940463 - JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars affects the variable lifetime or something in the system module

Status: RESOLVED INVALID

(Core :: JavaScript Engine, task)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 1881888 comment #2.

In bug 1881888, I'm about to flip the JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars option for system modules and then remove the option, but that hits an intermittent failure on browser_privatebrowsing_downloadLastDir_toggle.js.

The issue is that the gDownloadLastDir.file property is not reset to null there.

https://searchfox.org/mozilla-central/rev/f53c09a22edc700ab1a9eaaf4da0f0dd9f11bff3/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_toggle.js#43-45

gDownloadLastDir.file = tmpDir;
clearHistory();
is(gDownloadLastDir.file, null, "gDownloadLastDir.file should be null");

It's supposed to be reset by the clearHistory call:

https://searchfox.org/mozilla-central/rev/7d1b5c88343879056168aa710a9ee743392604c0/browser/components/privatebrowsing/test/browser/head.js#78-81

function clearHistory() {
  // simulate clearing the private data
  Services.obs.notifyObservers(null, "browser:purge-session-history");
}

Which triggers an observer notification for the observer in DownloadLastDir.sys.mjs.
If I put dump(...) call in the observer in DownloadLastDir.sys.mjs the dump is not performed for the problematic case,
which would mean either the observer is somehow removed, or the scope is somehow dead and the code inside the function doesn't work.

https://searchfox.org/mozilla-central/rev/7d1b5c88343879056168aa710a9ee743392604c0/toolkit/mozapps/downloads/DownloadLastDir.sys.mjs#45,51-52,56,74-76,79

var observer = {
...
  observe(aSubject, aTopic) {
    switch (aTopic) {
...
      case "browser:purge-session-history":
...
    }
  },
};
...
Services.obs.addObserver(observer, "browser:purge-session-history", true);

If I explicitly close over the observer variable in other function, the issue disappear, so this is an issue around the scope or the GC or some interaction between them.

Comment 1

[Tooru Fujisawa [:arai]]

actually, it's a bug in DownloadLastDir.sys.mjs.
It uses ownsWeak = true for the addObserver call, but nothing holds the reference.
So the object can be GCed in the regular situation.

https://searchfox.org/mozilla-central/rev/ead020d3989d3e9477b353d3d117f9c0f4b16f53/xpcom/ds/nsIObserverService.idl#32-39

 * @param ownsWeak  : If set to false, the nsIObserverService will hold a
 *                    strong reference to |anObserver|.  If set to true and
 *                    |anObserver| supports the nsIWeakReference interface,
 *                    a weak reference will be held.  Otherwise an error will be
 *                    returned.
 */
void addObserver( in nsIObserver anObserver, in string aTopic,
                  [optional] in boolean ownsWeak);

Here, deoptimizeModuleGlobalVars option marks the observer variable closed over, which puts the variable in the environment object, and that keeps the observer alive.

Anyway, given this is specific to nsIObserverService usage and the system module loader, I'll mark this bug as invalid,
and move the actual fix for the file in separate bug.