In bug 1451484, we're trying to convert our bootstrapped extension to a WebExtension, and one of the features we need is the ability to override user agents. So I decided to use browser.webRequest.onBeforeSendHeaders for that purpose, but unfortunately, I trigger leakchecks in some tests inside `dom/serviceworkers/test/`. I created a minimal testcase that triggers the tests, see the attached patch. A try run with this patch is at [0], the log for the failing leakcheck can be found at [1]: > TEST-INFO | leakcheck | default process: leaked 1 BackstagePass > TEST-INFO | leakcheck | default process: leaked 5 ChannelWrapper > TEST-INFO | leakcheck | default process: leaked 5 DOMEventTargetHelper > TEST-INFO | leakcheck | default process: leaked 1 ProtoAndIfaceCache > TEST-INFO | leakcheck | default process: leaked 5 WeakReference<ChannelWrapper> > TEST-INFO | leakcheck | default process: leaked 1 XPCNativeInterface > TEST-INFO | leakcheck | default process: leaked 1 XPCNativeMember > TEST-INFO | leakcheck | default process: leaked 1 XPCNativeSet > TEST-INFO | leakcheck | default process: leaked 1 XPCWrappedNative > TEST-INFO | leakcheck | default process: leaked 1 XPCWrappedNativeProto > TEST-INFO | leakcheck | default process: leaked 1 XPCWrappedNativeTearOff > TEST-INFO | leakcheck | default process: leaked 1 nsJSPrincipals > TEST-INFO | leakcheck | default process: leaked 11 nsTArray_base > TEST-UNEXPECTED-FAIL | leakcheck | default process: 3888 bytes leaked (BackstagePass, ChannelWrapper, DOMEventTargetHelper, ProtoAndIfaceCache, WeakReference<ChannelWrapper>, ...) I'm setting P1, as our patch to convert the bootstrapped extension has to be done as soon as possible as the team will start removing supported for bootstrapped extensions as soon as Nightly hits 65... So I'd appreciate some help. [0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5abfee1a8c0921fec66cceec78bbef69d64b4a31 [1]: https://taskcluster-artifacts.net/PNvhzJqCSlGgnmWJ91WCyQ/0/public/logs//log_raw.log

One thing I should add: When looking at the test logs, it's quite interesting that only the leakcheck at the end of the run fails, all leakchecks in between the tests seem to be passing. This could very well not be an actual memleak, but something else. But as this is happening inside WebExtension sources I am not familiar with, this is a bit out of my expertise.

David, is this a known issue? It's probably a blocker to 1451484... I can't imagine we don't get backed out if we land that with failing leakcheck tests. Thanks.

I've narrowed it down a bit to dom/serviceworkers/test/test_https_origin_after_redirect.html Among other things, this test uses this sjs script to send a 308 redirect: https://searchfox.org/mozilla-central/source/dom/serviceworkers/test/fetch/origin/https/index-https.sjs I'll try to keep digging but in the mean time, Shane: does the possibility of webrequest leaking while handling redirects ring any bells?

I don't think this is a real leak. It looks like all the ChannelWrappers being leaked are weakref'd. The extension would be loaded on startup (at some point), and unloaded after all tests are complete. I'm not sure how leakcheck works. My guess is that it snapshots what is running on startup and compares against a snapshot on shutdown. If the extension starts after leakcheck gets its baseline, and is unloaded (or certain things GCd) after leakcheck gets a shutdown snapshot, it would register as a leak. I would be surprised if this isn't intermittent in other tests.

(In reply to Shane Caraveo (:mixedpuppy) from comment #4) > I don't think this is a real leak. It looks like all the ChannelWrappers > being leaked are weakref'd. How did you conclude that? > The extension would be loaded on startup (at some point), and unloaded after > all tests are complete. I'm not sure how leakcheck works. My guess is that > it snapshots what is running on startup and compares against a snapshot on > shutdown. If the extension starts after leakcheck gets its baseline, and is > unloaded (or certain things GCd) after leakcheck gets a shutdown snapshot, > it would register as a leak. I'm not following your reasoning here, this is not leaking the extension page or anything like that, its just leaking (or reporting a leak of) a ChannelWrapper and some xpconnect gunk.

(In reply to Andrew Swan [:aswan] from comment #5) > (In reply to Shane Caraveo (:mixedpuppy) from comment #4) > > I don't think this is a real leak. It looks like all the ChannelWrappers > > being leaked are weakref'd. > > How did you conclude that? TEST-INFO | leakcheck | default process: leaked 5 ChannelWrapper TEST-INFO | leakcheck | default process: leaked 5 WeakReference<ChannelWrapper> In my own testing, those two are always the same. We don't keep a hard reference anywhere I know of, I'm assuming the first line is due to the second line. WebRequestService maintains a list of ChannelWrapper, but those are held by a WeakPtr. > > The extension would be loaded on startup (at some point), and unloaded after > > all tests are complete. I'm not sure how leakcheck works. My guess is that > > it snapshots what is running on startup and compares against a snapshot on > > shutdown. If the extension starts after leakcheck gets its baseline, and is > > unloaded (or certain things GCd) after leakcheck gets a shutdown snapshot, > > it would register as a leak. > > I'm not following your reasoning here, this is not leaking the extension > page or anything like that, its just leaking (or reporting a leak of) a > ChannelWrapper and some xpconnect gunk. The extension is unloaded after the tests run, I'm just not clear if it is unloading after leakcheck. If it is unloaded after leakcheck, and it is keeping a reference somewhere, that would be why. Again, I'm not clear about how leakcheck works, so all this is just conjecture.

The gross contortions here are required to deal with the deferred finalizers HTTP channels use for their property bags. The actual channels get destroyed relatively early during shutdown, but their property bag hashes which hold our ChannelWrapper reference end up being destroyed after JS engine shutdown, which gives us no good point to clear our reference. The stub holder class takes the place of our existing property bag entry, and behaves more or less the same, but allows us to cut the reference to the ChannelWrapper without having a strong reference to the channel.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bdbb72ad61b6272b3b7faed01383bb7c5fe7e72 Bug 1495748: Make sure ChannelWrappers get cleaned up before JS engine shutdown. r=aswan,mccr8