Leak-until-shutdown with deviceorientation and unload listeners

RESOLVED FIXED in mozilla8



DOM: Events
6 years ago
6 years ago


(Reporter: Jesse Ruderman, Assigned: dougt)


(Blocks: 1 bug, {mlk, testcase})

mlk, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox6 affected)


(Whiteboard: [MemShrink:P3])


(2 attachments, 2 obsolete attachments)



6 years ago
Created attachment 543703 [details]

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.
Whiteboard: [MemShrink]

Comment 1

6 years ago
thanks for finding this.  I'll look this week.
Assignee: nobody → doug.turner
status-firefox6: --- → affected
Whiteboard: [MemShrink] → [MemShrink:P3]
Version: 1.9.2 Branch → unspecified

Comment 2

6 years ago
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.
Attachment #544522 - Flags: review?(Olli.Pettay)

Comment 3

6 years ago
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

6 years ago
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.
Attachment #544522 - Attachment is obsolete: true
Attachment #544726 - Flags: review?(Olli.Pettay)
Attachment #544522 - Flags: review?(Olli.Pettay)

Comment 5

6 years ago
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.
Attachment #544726 - Flags: review?(Olli.Pettay) → review-

Comment 6

6 years ago
Created attachment 545210 [details] [diff] [review]
patch v.3
Attachment #544726 - Attachment is obsolete: true
Attachment #545210 - Flags: review?(Olli.Pettay)

Comment 7

6 years ago
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.
Attachment #545210 - Flags: review?(Olli.Pettay) → review+

Comment 8

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

Last Resolved: 6 years ago
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Version: unspecified → Trunk


6 years ago
Depends on: 674156
You need to log in before you can comment on or make changes to this bug.