738774 - [Page Thumbnails] Channel leaks intermittently

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

The PageThumbs channel is leaking intermittently and that's the cause of bug 735574. I wasn't able to find out what the exact cause of the leak was but looking at the HttpChannel implementation we're doing too much work anyway at the moment.

I rewrote the Channel from scratch, guided by the HttpChannel implementation, using an InputStreamPump to deliver the image data. The leak isn't reproducible for me anymore. The try run doesn't show any leaks well: https://tbpl.mozilla.org/?tree=Try&rev=a06133ba8471

Comment 1

[Dietrich Ayala (:dietrich)]

Comment on attachment 608846 [details] [diff] [review]
patch v1

Review of attachment 608846 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, yeah a pump is better here. the channel+request in a single object is always spaghetti, but i didn't see anything obviously wrong.

i'm not super happy that we still don't know exactly what was happening, but let's roll with it to fix the leak for now.

::: browser/components/thumbnails/PageThumbsProtocol.js
@@ +165,5 @@
> +
> +    function trackThumbnailHitOrMiss(aThumbnailFound) {
> +      Services.telemetry.getHistogramById("FX_THUMBNAILS_HIT_OR_MISS")
> +        .add(aThumbnailFound);
> +    }

for clarity, should either have this as a method of the channel, or at least defined before use.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dietrich Ayala (:dietrich) from comment #1)
> i'm not super happy that we still don't know exactly what was happening, but
> let's roll with it to fix the leak for now.

I could imagine that maybe the asyncCopy() callback kept things alive and now that we can just cancel the pump there is nothing alive anymore.

> > +    function trackThumbnailHitOrMiss(aThumbnailFound) {
> > +      Services.telemetry.getHistogramById("FX_THUMBNAILS_HIT_OR_MISS")
> > +        .add(aThumbnailFound);
> > +    }
> 
> for clarity, should either have this as a method of the channel, or at least
> defined before use.

Moved to its own method.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/50483ab04fd6

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/50483ab04fd6

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 608846 [details] [diff] [review]
patch v1

[Approval Request Comment]
User impact if declined: possible leaks when showing thumbnails (i.e. opening the newtab page)
Testing completed (on m-c, etc.): landed on m-c ~3 days ago, no regressions so far
Risk to taking this patch (and alternatives if risky): risk seems low, we can't say for sure that those were "real" leaks occurring in the wild
String changes made by this patch: none

Comment 6

[Alex Keybl [:akeybl]]

I'm having a hard time weighing 

(In reply to Tim Taubert [:ttaubert] from comment #5)
> User impact if declined: possible leaks when showing thumbnails (i.e.
> opening the newtab page)
> Risk to taking this patch (and alternatives if risky): risk seems low, we
> can't say for sure that those were "real" leaks occurring in the wild
> String changes made by this patch: none

against the fact that this patch was a complete rewrite of the PageThumbs channel. I know we can't say whether users are running into this, but is there any way to reason about the worst this memory leak could end up being in normal user scenarios?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

I'm not really confident about what caused the leak but I *think* it was caused by using NetUtils.asyncCopy() which held the callback/closure alive even though the image request has been cancelled already. The leak was frequent but still intermittent so I think it was only because the code was kept alive a little longer than it should've been. I'm talking about probably less than 5 seconds only when quickly opening and closing the new tab page.

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> I'm talking about
> probably less than 5 seconds only when quickly opening and closing the new
> tab page.

Sorry, what's the amount of memory leaked each time a user opens the new tab page in our tests?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

So my guess is that we could be leaking ~46kb when quickly opening and closing the newtab page for some seconds only. We don't leak when opening the newtab page - it's about the images being loaded. When they're not loaded completely before closing the page we leak them until they're loaded (which should only be a second or two, worst). If we shut down the browser while these are still loading we have our intermittent leak.

I'd probably not backport this fix for Fx 13 as I don't think this will really affect users in the wild and may not be worth the risk.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 608846 [details] [diff] [review]
patch v1

[Triage Comment]
I agree with Tim's analysis that this fix is likely unnecessary for FF13, and carries greater than usual risk based upon the fact that this is a code rewrite.