Leak-until-shutdown with deviceorientation and unload listeners

RESOLVED FIXED in mozilla8

Status

()

Core
DOM: Events
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: dougt)

Tracking

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

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

Firefox Tracking Flags

(firefox6 affected)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Whiteboard: [MemShrink]
(Assignee)

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
(Assignee)

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.
(Assignee)

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-
(Assignee)

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+
(Assignee)

Comment 8

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/d734e168a228
Merged:
http://hg.mozilla.org/mozilla-central/rev/d734e168a228
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Version: unspecified → Trunk
(Reporter)

Updated

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