Leak with expando on navigator.mediaDevices

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jruderman, Assigned: mccr8)

Tracking

(Blocks 1 bug, {memory-leak, testcase})

Trunk
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox39 fixed, firefox40 fixed, firefox41 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

Posted file testcase
Running with XPCOM_MEM_LEAK_LOG=2 shows leaked nsDocument and nsGlobalWindow.
Assignee: nobody → continuation
Whiteboard: [MemShrink]
CC logs showed that it was a DETH that was being holding things alive. The DETH expando indicated that it was for a MediaDevice object, which is being held alive by the Navigator object, but not traversed or unlinked.  Poiru, here's an example of something that a CC static analysis should detect. ;)
The test case itself also pretty obviously points at Navigator, now that I am looking at it.  Anyways, this is an example of the sort of leak (stick an expando on an object you'd never normally do that on!) that we only really notice with fuzzing.
Jesse, do you remember why you marked bug 1046245 as the regressor?  From code inspection, it looks to me like bug 1033885 would have caused this.  It doesn't matter much either way, I'm just curious.
Flags: needinfo?(jruderman)
Trivial patch, with test.  I didn't add mMediaDevices to Invalidate(), because we don't really need to explicitly clear it in the dtor.

try run just in case:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7e9e017d4a3
Attachment #8607226 - Flags: review?(bugs)
Comment on attachment 8607226 [details] [diff] [review]
Tell the cycle collector about Navigator::mMediaDevices.

For consistency, could we perhaps do the unlinking in
::Invalidate(). If we want to start to keeping stuff alive longer, we could then
change all that stuff.

With that r+
Attachment #8607226 - Flags: review?(bugs) → review+
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Jesse, do you remember why you marked bug 1046245 as the regressor?  From
> code inspection, it looks to me like bug 1033885 would have caused this.  It
> doesn't matter much either way, I'm just curious.

I just marked the bug as "blocking the affected feature", without checking whether the bug existed back to its introduction. Is that something I should stop doing?
Flags: needinfo?(jruderman)
(In reply to Jesse Ruderman from comment #6)
> I just marked the bug as "blocking the affected feature", without checking
> whether the bug existed back to its introduction.
That makes sense. I don't really understand the ordering of the landing of those two bugs, but I guess it doesn't matter. ;)

> Is that something I should stop doing?

No, I think it is a good idea, I was just trying to figure out if my analysis might be wrong. I mean, it still might be.
(In reply to Olli Pettay [:smaug] from comment #5)
> For consistency, could we perhaps do the unlinking in
> ::Invalidate().
Done.
https://hg.mozilla.org/mozilla-central/rev/900680d18e50
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8607226 [details] [diff] [review]
Tell the cycle collector about Navigator::mMediaDevices.

Approval Request Comment
[Feature/regressing bug #]: bug 1046245 or maybe something older
[User impact if declined]: severe leaks if web pages do something weird
[Describe test coverage new/current, TreeHerder]: I added a test for the leak, and there is existing testing for the stuff this modified.
[Risks and why]: Very low. Simple patch.
[String/UUID change made/needed]: None.
Attachment #8607226 - Flags: approval-mozilla-beta?
Attachment #8607226 - Flags: approval-mozilla-aurora?
Comment on attachment 8607226 [details] [diff] [review]
Tell the cycle collector about Navigator::mMediaDevices.

Approved for uplift to aurora and beta. Has tests, fixes a leak; in theory this is just what we want to be fixing and uplifting to beta!
Attachment #8607226 - Flags: approval-mozilla-beta?
Attachment #8607226 - Flags: approval-mozilla-beta+
Attachment #8607226 - Flags: approval-mozilla-aurora?
Attachment #8607226 - Flags: approval-mozilla-aurora+
This bug wins the "Poetic Bugzilla Award of the Day for "a DETH that was being holding things alive".
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.