Last Comment Bug 669105 - Leak-until-shutdown with deviceorientation and unload listeners
: Leak-until-shutdown with deviceorientation and unload listeners
Status: RESOLVED FIXED
[MemShrink:P3]
: mlk, testcase
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Doug Turner (:dougt)
:
:
Mentors:
Depends on: 674156
Blocks: 325861
  Show dependency treegraph
 
Reported: 2011-07-03 15:51 PDT by Jesse Ruderman
Modified: 2011-07-25 21:33 PDT (History)
6 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected


Attachments
testcase (291 bytes, text/html)
2011-07-03 15:51 PDT, Jesse Ruderman
no flags Details
patch v.1 (4.84 KB, patch)
2011-07-07 09:44 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.2 (9.43 KB, patch)
2011-07-07 21:36 PDT, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch v.3 (9.51 KB, patch)
2011-07-11 10:55 PDT, Doug Turner (:dougt)
bugs: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-07-03 15:51:59 PDT
Created attachment 543703 [details]
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) 2011-07-04 00:05:55 PDT
thanks for finding this.  I'll look this week.
Comment 2 Doug Turner (:dougt) 2011-07-07 09:44:28 PDT
Created attachment 544522 [details] [diff] [review]
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] (reviewing overload) 2011-07-07 09:59:39 PDT
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) 2011-07-07 21:36:23 PDT
Created attachment 544726 [details] [diff] [review]
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] (reviewing overload) 2011-07-08 03:32:28 PDT
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) 2011-07-11 10:55:59 PDT
Created attachment 545210 [details] [diff] [review]
patch v.3
Comment 7 Olli Pettay [:smaug] (reviewing overload) 2011-07-11 11:20:05 PDT
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) 2011-07-11 23:15:54 PDT
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) 2011-07-12 03:59:21 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/d734e168a228

Note You need to log in before you can comment on or make changes to this bug.