Last Comment Bug 738774 - [Page Thumbnails] Channel leaks intermittently
: [Page Thumbnails] Channel leaks intermittently
Status: RESOLVED FIXED
[MemShrink:P1]
: mlk
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on:
Blocks: 735574
  Show dependency treegraph
 
Reported: 2012-03-23 13:23 PDT by Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
Modified: 2012-04-09 14:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (15.37 KB, patch)
2012-03-23 13:23 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dietrich: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-23 13:23:02 PDT
Created attachment 608846 [details] [diff] [review]
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) 2012-03-26 16:23:53 PDT
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] (on PTO, back Aug 29th) 2012-03-27 05:08:13 PDT
(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] (on PTO, back Aug 29th) 2012-03-27 05:16:15 PDT
https://hg.mozilla.org/integration/fx-team/rev/50483ab04fd6
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-27 15:38:26 PDT
https://hg.mozilla.org/mozilla-central/rev/50483ab04fd6
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-30 03:36:30 PDT
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] 2012-04-02 10:42:11 PDT
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] (on PTO, back Aug 29th) 2012-04-03 05:29:29 PDT
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] 2012-04-04 16:02:24 PDT
(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] (on PTO, back Aug 29th) 2012-04-05 03:21:42 PDT
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] 2012-04-09 14:41:12 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.