Closed Bug 1359935 Opened 3 years ago Closed 3 years ago

Removing GC call in nsXREDirProvider::DoShutdown() leaks while running dom/presentation chrome mochitests

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file)

No description provided.
Four .jsms leak:
  chrome://specialpowers/content/SpecialPowersObserver.jsm
  file:///home/amccreight/mc/obj-dbg.noindex/dist/bin/components/multiprocessShims.js
  resource://gre/modules/RemoteAddonsParent.jsm
  resource://gre/modules/XPCOMUtils.jsm
The leak occurs specifically with the test test_presentation_sender_startWithDevice.html
This seems to be a bug with registerOriginalFactory in PresentationSessionChromeScript.js. This class sets up a number of mock factories for testing purposes, but it only unregisters those that are replacing an existing factory. Changing this so we unregister all of the factories (but only re-register when there was an existing factory) seems to fix the leak.
See Also: → 1359958
My guess would be that this doesn't leak when the JS_GC is present because something very late in shutdown shakes out the factories, and then this final GC is somehow enough to cause the jsms to go away.
Never mind, my patch does not actually fix anything. Need to investigate more.
I think I have an actual fix now: for some reason, mockedSessionTransport.close() gets called after the special powers stuff has been torn down, and somehow calling sendAsyncMessage then causes a leak. I avoid this by not doing anything in close() if callback_ is null, which indicates that tearDown has been called already.
Comment on attachment 8863059 [details]
Bug 1359935 - Don't call sendAsyncMessage in mockedSessionTransport.close() after tearDown.

https://reviewboard.mozilla.org/r/134896/#review138256

Looks good.

Thanks.
Attachment #8863059 - Flags: review?(kechang) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/758e3a7da9bd
Don't call sendAsyncMessage in mockedSessionTransport.close() after tearDown. r=kershaw
https://hg.mozilla.org/mozilla-central/rev/758e3a7da9bd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
This is a test-only change that will be needed on beta if we are going to eventually uplift bug 833143 to beta to improve a shutdown hang.
Keywords: checkin-needed
Whiteboard: [checkin-needed Beta]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.