Closed
Bug 1157995
Opened 9 years ago
Closed 9 years ago
Leak with expando on navigator.mediaDevices
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: jruderman, Assigned: mccr8)
References
Details
(Keywords: memory-leak, testcase, Whiteboard: [MemShrink])
Attachments
(2 files)
71 bytes,
text/html
|
Details | |
2.62 KB,
patch
|
smaug
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Running with XPCOM_MEM_LEAK_LOG=2 shows leaked nsDocument and nsGlobalWindow.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → continuation
Whiteboard: [MemShrink]
Assignee | ||
Comment 1•9 years ago
|
||
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. ;)
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
status-firefox39:
--- → affected
status-firefox41:
--- → affected
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Reporter | ||
Comment 6•9 years ago
|
||
(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)
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
(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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
This bug wins the "Poetic Bugzilla Award of the Day for "a DETH that was being holding things alive".
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7fafcb9f12e8
Flags: in-testsuite+
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•