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

Status: RESOLVED FIXED

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

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

The leak occurs specifically with the test test_presentation_sender_startWithDevice.html

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Andrew McCreight [:mccr8]]

Never mind, my patch does not actually fix anything. Need to investigate more.

Comment 6

[Andrew McCreight [:mccr8]]

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 7

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=686633edd3a68ac6eeb8e10dc0e4e3532a0bbce1

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Bug 1359935 - Don't call sendAsyncMessage in mockedSessionTransport.close() after tearDown.

For some reason, when running test_presentation_sender_startWithDevice.html
we end up running mockedSessionTransport.close() late in shutdown. The
special powers code to do sendAsyncMessage has been torn down already,
and we end up leaking in shutdown, without an extra GC.

Also, this patch replaces some leading tabs with spaces.

Review commit: https://reviewboard.mozilla.org/r/134896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134896/

Comment 9

[Kershaw Chang [:kershaw]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/758e3a7da9bd
Don't call sendAsyncMessage in mockedSessionTransport.close() after tearDown. r=kershaw

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/758e3a7da9bd

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b6126c6706d0