Closed Bug 1260528 Opened 4 years ago Closed 4 years ago

Tests for notifications that pass an iconUrl are causing leaks

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
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.
No longer depends on: 1254291
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|
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.
It should be something along the lines of:

Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
    .getImgCacheForDocument(null)
    .clearCache(true);
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.2 - Apr 4
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)
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.
Depends on: 1261231
Thanks Kris. I'll update the tests once that bug is fixed.
Iteration: 48.2 - Apr 4 → 48.3 - Apr 18
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)
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 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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e212ede37388
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.