Closed
Bug 1260528
Opened 8 years ago
Closed 8 years ago
Tests for notifications that pass an iconUrl are causing leaks
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bsilverberg, Assigned: bsilverberg)
References
Details
(Whiteboard: [notifications])
Attachments
(1 file)
This was uncovered during bug 1254291, and it was decided to land that bug with non-leaking tests by not passing an `iconUrl` into the `notifications.create` method. We should also have tests that do pass a valid iconUrl into notifications.create, but we will need to deal with the leaks.
Assignee | ||
Comment 1•8 years ago
|
||
Copied from https://bugzilla.mozilla.org/show_bug.cgi?id=1254291#c12: FTR, the leaks produced (which I can reproduce locally with a debug build) are: |<----------------Class--------------->|<-----Bytes------>|<----Objects---->| | | Per-Inst Leaked| Total Rem| 0 |TOTAL | 34 416| 1223625 5| 349 |Mutex | 32 32| 1077 1| 690 |imgLoader | 336 336| 3 1| 691 |imgMemoryReporter | 32 32| 1 1| 1159 |nsStringBuffer | 8 8| 105412 1| 1199 |nsTArray_base | 8 8| 286137 1|
Assignee | ||
Comment 2•8 years ago
|
||
It was suggested by Kris that we might fix the leaks by flushing the image cache, but I'm not sure how to do that. If someone can provide a clue about where to look to figure out how to do that I can investigate and give it a try.
Comment 3•8 years ago
|
||
It should be something along the lines of: Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools) .getImgCacheForDocument(null) .clearCache(true);
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.2 - Apr 4
Assignee | ||
Comment 4•8 years ago
|
||
This doesn't address the leaks but I wanted to present what I've tried for comments. I tried both `imageCache.clearCache(true)` and `imageCache.clearCache(false)`, and I also tried adding the code to clear the cache in both a `SimpleTest.registerCleanupFunction` call and also inline in the test immediately before `yield extension.unload()`. Not matter what combination I tried the leaks were always the same, which is to say the same as what they were before the cache clearing code was added. Review commit: https://reviewboard.mozilla.org/r/43541/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43541/
Attachment #8736771 -
Flags: review?(kmaglione+bmo)
Comment 5•8 years ago
|
||
It turns out this is being caused by a reference counting bug in the imgLoader code. I'll fix that in a separate bug, and then we should be OK to just turn on the test with the image URL.
Assignee | ||
Comment 6•8 years ago
|
||
Thanks Kris. I'll update the tests once that bug is fixed.
Assignee | ||
Updated•8 years ago
|
Iteration: 48.2 - Apr 4 → 48.3 - Apr 18
Comment 7•8 years ago
|
||
Comment on attachment 8736771 [details] MozReview Request: Bug 1260528 - Tests for notifications that pass an iconUrl are causing leaks, r?kmag https://reviewboard.mozilla.org/r/43541/#review42411 ::: toolkit/components/extensions/test/mochitest/test_ext_notifications.html:18 (Diff revision 1) > "use strict"; > > +// A 1x1 PNG image. > +// Source: https://commons.wikimedia.org/wiki/File:1x1.png (Public Domain) > +const IMAGE = atob("iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAA" + > + "ACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII="); This needs to be converted to an `ArrayBuffer`. Also, please align the second line after the opening paren. ::: toolkit/components/extensions/test/mochitest/test_ext_notifications.html:25 (Diff revision 1) > +SimpleTest.registerCleanupFunction(() => { > + let imageCache = SpecialPowers.Cc["@mozilla.org/image/tools;1"] > + .getService(SpecialPowers.Ci.imgITools) > + .getImgCacheForDocument(null); > + imageCache.clearCache(true); > +}); We shouldn't actually need to clear the cache once the other issue is fixed.
Attachment #8736771 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8736771 [details] MozReview Request: Bug 1260528 - Tests for notifications that pass an iconUrl are causing leaks, r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43541/diff/1-2/
Attachment #8736771 -
Flags: review?(kmaglione+bmo)
Comment 9•8 years ago
|
||
Comment on attachment 8736771 [details] MozReview Request: Bug 1260528 - Tests for notifications that pass an iconUrl are causing leaks, r?kmag https://reviewboard.mozilla.org/r/43541/#review43513
Attachment #8736771 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82f83d0ec43b
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e212ede37388
Keywords: checkin-needed
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e212ede37388
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•