1940741 - DownloadLastDir.sys.mjs should use a strong ref for the observer

Status: RESOLVED FIXED

(Toolkit :: Downloads API, task)

Description

[Tooru Fujisawa [:arai]]

(Discovered by bug 1940463)

DownloadLastDir.sys.mjs adds observer with ownsWeak=true.

https://searchfox.org/mozilla-central/rev/ead020d3989d3e9477b353d3d117f9c0f4b16f53/toolkit/mozapps/downloads/DownloadLastDir.sys.mjs#45,78-79

var observer = {
...
Services.obs.addObserver(observer, "last-pb-context-exited", true);
Services.obs.addObserver(observer, "browser:purge-session-history", true);

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);

The observer is not held by anything explicitly, but the variable is held by JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars option, which marks all global variables closed over, which puts them into the module environment object.
So, the lifetime of the observer mostly accidentally becomes equals to the module.

bug 1881888 is going to flip the option, and the option is going to be removed.
And the observer is no longer strongly held by anything, and that results in hitting assertion failure in the following, due to the observer getting GCed.

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");

We should instead use strong reference, with explicitly removing the observers at "shutdown-leaks-before-check" notification or something.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Bug 1940741 - Remove XPCOMUtils.defineLazyModuleGetters. r?mccr8!

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1940741 - Do not use weak ref for the observer in DownloadLastDir.sys.mjs. r?Gijs!

Comment 3

[Phabricator Automation]

Comment on attachment 9446615 [details]
Bug 1940741 - Remove XPCOMUtils.defineLazyModuleGetters. r?mccr8!

Revision D233379 was moved to bug 1940422. Setting attachment 9446615 [details] to obsolete.

Comment 4

[Marco Bonardo [:mak]]

(In reply to Tooru Fujisawa [:arai] from comment #0)

So, the lifetime of the observer mostly accidentally becomes equals to the module.

Afaict, that was exactly the scope here.
Stopping observing at shutdown would be fine, both of those topics can happen quite late on shutdown, the history clear one can likely happen at profile-before-change, I'm not sure about last-pb-context-exited, though I suspect we can't use anything earlier than xpcom-will-shutdown

bug 1881888 is going to flip the option, and the option is going to be removed.

Which option?

And the observer is no longer strongly held by anything, and that results in hitting assertion failure in the following, due to the observer getting GCed.

That's quite an interesting change in how modules globals used to work, I wonder if it may cause other subtle bugs. It may be worth checking if there's other cases similar to this one.

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Marco Bonardo [:mak] from comment #4)

Stopping observing at shutdown would be fine, both of those topics can happen quite late on shutdown, the history clear one can likely happen at profile-before-change, I'm not sure about last-pb-context-exited, though I suspect we can't use anything earlier than xpcom-will-shutdown

Okay, I'll look into xpcom-will-shutdown and xpcom-shutdown

bug 1881888 is going to flip the option, and the option is going to be removed.

Which option?

it's the JS::TransitiveCompileOptions.deoptimizeModuleGlobalVars option, which is currently set here:

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/js/xpconnect/loader/mozJSModuleLoader.cpp#1139-1140,1146-1147

/* static */
void mozJSModuleLoader::SetModuleOptions(CompileOptions& aOptions) {
...
  // Make all top-level `vars` available in `ModuleEnvironmentObject`.
  aOptions.deoptimizeModuleGlobalVars = true;

And the observer is no longer strongly held by anything, and that results in hitting assertion failure in the following, due to the observer getting GCed.

That's quite an interesting change in how modules globals used to work, I wonder if it may cause other subtle bugs. It may be worth checking if there's other cases similar to this one.

If this behavior has been intentionally used for lifetime management with weak references, then yes, we should check other cases.
I've skimmed other observers with ownsWeak=true parameter, and didn't find any other problematic cases, but I might have missed something.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Tooru Fujisawa [:arai] from comment #5)

If this behavior has been intentionally used for lifetime management with weak references, then yes, we should check other cases.

I'm pretty sure the scope here was just to have this observer alive until the module was alive, that was supposed to be for the lifetime of the process.

Comment 7

[Tooru Fujisawa [:arai]]

So far there are several objects and classed with nsISupportsWeakReference interface in sys.mjs files.
I'll check each of them to see if any instance is held by global variable.

https://searchfox.org/mozilla-central/search?q=nsISupportsWeakReference&path=.sys.mjs&case=false&regexp=false

Comment 8

[Tooru Fujisawa [:arai]]

Here's the list of objects with nsISupportsWeakReference interface, stored into global var.
All of those variables are closed over, so they're not affected by the deoptimizeModuleGlobalVars option, but still risky.

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/browser/actors/RefreshBlockerChild.sys.mjs#15,127-132

var progressListener = {
...
  QueryInterface: ChromeUtils.generateQI([
    "nsIWebProgressListener2",
    "nsIWebProgressListener",
    "nsISupportsWeakReference",
  ]),
};

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/browser/components/downloads/DownloadsCommon.sys.mjs#93-97

var PrefObserver = {
  QueryInterface: ChromeUtils.generateQI([
    "nsIObserver",
    "nsISupportsWeakReference",
  ]),

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/dom/media/PeerConnection.sys.mjs#209-213,215-217

setupPrototype(GlobalPCList, {
  QueryInterface: ChromeUtils.generateQI([
    "nsIObserver",
    "nsISupportsWeakReference",
  ]),
...
});

var _globalPCList = new GlobalPCList();

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/toolkit/components/satchel/FormHistory.sys.mjs#104,175-179

var Prefs = {
...
  QueryInterface: ChromeUtils.generateQI([
    "nsIObserver",
    "nsISupportsWeakReference",
  ]),
};

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs#283,1115

var Impl = {
...
  QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"]),

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/toolkit/components/telemetry/app/TelemetryEnvironment.sys.mjs#85,88,1047,1315,2169

var gGlobalEnvironment;
...
    gGlobalEnvironment = new EnvironmentCache();
...
EnvironmentCache.prototype = {
...
  QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"]),
...
};

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/toolkit/modules/NewTabUtils.sys.mjs#207,308-312

var AllPages = {
...
  QueryInterface: ChromeUtils.generateQI([
    "nsIObserver",
    "nsISupportsWeakReference",
  ]),
};

https://searchfox.org/mozilla-central/rev/6d06ffc186ac356648f1feeadd52cc61b2ad6737/toolkit/modules/NewTabUtils.sys.mjs#1637,2137-2141

var Links = {
...
  QueryInterface: ChromeUtils.generateQI([
    "nsIObserver",
    "nsISupportsWeakReference",
  ]),
};

Comment 9

[Andrew McCreight [:mccr8]]

It sounds like you have already moved away from it, but please don't use "shutdown-leaks-before-check" to clear references. That will hide leaks from our leak checker.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Tooru Fujisawa [:arai] from comment #8)

All of those variables are closed over

What's the difference from the observer object? what keeps progressListener alive, for example? Thanks, I feel like I'm not grasping the whole story around deoptimizeModuleGlobalVars and what makes these cases different.

Comment 11

[Tooru Fujisawa [:arai]]

(In reply to Marco Bonardo [:mak] from comment #10)

(In reply to Tooru Fujisawa [:arai] from comment #8)

All of those variables are closed over

What's the difference from the observer object? what keeps progressListener alive, for example? Thanks, I feel like I'm not grasping the whole story around deoptimizeModuleGlobalVars and what makes these cases different.

observer variable is not closed over by any inner function, which means the observer variable won't be accessed once the top-level script finishes executing.
In this case, with the standard ES module, the variable is allocated in the stack-allocated call frame, and the pointed object is strongly held only while the frame is on the stack.
So, the object can be GCed shortly after the module is imported, and nothing guarantees the observer notification callback being executed.

progressListener variable is closed over by 2 inner functions (RefreshBlockerObserverChild#enable and RefreshBlockerObserverChild#disable), which means the progressListener variable is used whenever the inner functions are called, even after the top-level script finishes executing.
In this case, the variable is allocated in the heap-allocated environment object, and the pointed object is strongly held as long as the environment object is alive. Here, the environment object has the same lifetime as the module.
So, the object won't be GCed and the callback for this is called as long as the module is alive.

Then, the current .sys.mjs puts all variables in the environment object (which is controlled by deoptimizeModuleGlobalVars option), because of the "JSM to ESM shim", and the Cu.import requires accessing all global variables regardless whether it's exported or not.
This changes the situation for the former observer case, and the object has the same lifetime as the module.
So, the behavior difference of the global variable between JSM and the standard ES module was overlooked when we migrate.

Now we're going to remove all the JSM loader and the JSM to ESM shim, the above behavior introduced by deoptimizeModuleGlobalVars should no longer be necessary, and I wanted to flip it, so that the global variables' behavior aligns with the standard ES module.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c132d9d4c09
Do not use weak ref for the observer in DownloadLastDir.sys.mjs. r=Gijs,mak

Comment 13

[Serban Stanca [:SerbanS]]

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