669105 - Leak-until-shutdown with deviceorientation and unload listeners

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Run a debug build so you can watch for --DOMWINDOW lines.
2. Load the testcase, which redirects itself.
3. Load about:memory.
4. Click "minimize memory use".

Result: the domwindow corresponding to the testcase does not go away.

Comment 1

[Doug Turner (:dougt)]

thanks for finding this.  I'll look this week.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

basically there is a race.  We are starting up the device motion system on a window that has already been closed.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 544522 [details] [diff] [review]
patch v.1

I don't understand why this fixes the leak.
SetHasOrientationEventListener is called when devicemotion listener
is added. At that point the window should still have
docshell since the loading of "data:text/html,2"; hasn't started.

Btw, I'd like to see the leak test to be put in an iframe.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

smaug and I discussed this a bit, and we decided that the best option was to make the DeviceMotion class hold weak references to the nsGlobalWindow.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 544726 [details] [diff] [review]
patch v.2

Sorry, but I think the nsTPtrArray approach is better, and simpler. No need
purge anything.

nsCOMArray<nsIDOMWindow> -> nsTPtrArray<nsIDOMWindow>
and the make Add/RemoveWindowListener [noscript] and document that
it is up to the caller to call RemoveWindowListener before the window is deleted.
Then add RemoveWindowListener call to the nsGlobalWindow dtor.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 545210 [details] [diff] [review]
patch v.3

>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -1891,16 +1891,19 @@ nsGlobalWindow::SetNewDocument(nsIDocume
> 
>     if (NS_FAILED(rv) || !equal) {
>       // Different origins.  Release the navigator object so it gets
>       // recreated for the new document.  The plugins or mime types
>       // arrays may have changed. See bug 150087.
>       mNavigator->SetDocShell(nsnull);
> 
>       mNavigator = nsnull;
>+
>+      // Also kill any device motion
>+      DisableDeviceMotionUpdates();
>     }
>   }

I don't understand this change.
This code runs only in the outer window, but EnableDeviceMotionUpdates is called
only on inner window.
So, as far as I see, this is just useless change.

Fortunately there is DisableDeviceMotionUpdates() in CleanUp since that
is called also in dtor.
Please add an assertion to the end of dtor that !mHasDeviceMotion.

Comment 8

[Doug Turner (:dougt)]

fixed things up.  Also removed existence checks on adding and removing a window listener.

http://hg.mozilla.org/integration/mozilla-inbound/rev/d734e168a228

Comment 9

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/d734e168a228