609752 - Leaks when reloading HTTP images

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alon Zakai (:azakai)]

Attachment: example html file

1. Copy a.html and img.png into a directory
2. Apply http://hg.mozilla.org/users/azakai_mozilla.com/patches/file/d17a187b31b5/ipc_profiler (needed for output redirection of processes) and build Fennec.
3. Run        python -m SimpleHTTPServer 8989         (a webserver)
4. Run        MOZ_REDIRECT_CHILD_STDOUT=co MOZ_REDIRECT_CHILD_STDERR=ce XPCOM_MEM_LEAK_LOG=1 ./fennec a.html ; gedit co
5. After the page loads, press reload.
6. Shut down, see a leak in the file 'co'.

This does not happen if

 * There is no image on the page
 * You do not reload
 * The page is not loaded via http (e.g., loading it with file://)

The leak looks something like this:

== BloatView: ALL (cumulative) LEAK STATISTICS, tab process 7706

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          24     4874    31520      101 (  656.27 +/-   898.13)    66097       98 ( 1197.99 +/-  2328.28)
  36 HttpChannelChild                              348      348        4        1 (    1.71 +/-     0.76)      239        1 (   11.14 +/-     3.51)
  56 PHttpChannelChild                              24       24        4        1 (    1.71 +/-     0.76)        0        0 (    0.00 +/-     0.00)
  73 RasterImage                                   140      140        4        1 (    2.29 +/-     1.11)       46        2 (    7.99 +/-     2.55)
  79 StringAdopt                                     1        2      440        2 (    3.22 +/-     0.98)        0        0 (    0.00 +/-     0.00)
 101 gfxASurface                                    20       20       36        1 (    6.62 +/-     2.14)        0        0 (    0.00 +/-     0.00)
 115 imgRequest                                    156      156        4        1 (    2.29 +/-     1.11)       71        1 (    9.35 +/-     3.52)
 117 imgRequestProxy                                64       64        5        1 (    2.78 +/-     1.39)      160        1 (   13.97 +/-     4.87)
 133 nsBaseURLParser                                12       12        3        1 (    1.80 +/-     0.84)     1592        4 (  166.94 +/-    95.83)
 181 nsCategoryObserver                             64       64        2        1 (    1.33 +/-     0.58)       30        1 (    8.47 +/-     3.40)
 197 nsDNSService                                   48       48        1        1 (    1.00 +/-     0.00)       35        1 (    9.67 +/-     5.10)
 226 nsDocShell::InterfaceRequestorProxy            16       16        1        1 (    1.00 +/-     0.00)       34        1 (    3.34 +/-     1.29)
 282 nsHashtable                                    44       44       20        1 (   10.26 +/-     5.71)        0        0 (    0.00 +/-     0.00)
 299 nsHttpConnectionInfo                           40       40        4        1 (    1.71 +/-     0.76)        4        1 (    1.71 +/-     0.76)
 300 nsHttpConnectionMgr                           100      100        1        1 (    1.00 +/-     0.00)        2        1 (    1.33 +/-     0.58)
 301 nsHttpHandler                                 316      316        1        1 (    1.00 +/-     0.00)      130        1 (    3.34 +/-     1.24)
 303 nsIDNService                                   64       64        1        1 (    1.00 +/-     0.00)       22        2 (    2.45 +/-     0.94)
 304 nsIOService                                   124      124        1        1 (    1.00 +/-     0.00)     1060        1 (    7.49 +/-     2.26)
 347 nsObserverService                              48       48        1        1 (    1.00 +/-     0.00)      202        1 (    9.76 +/-     3.70)
 356 nsPrefBranch                                   80       80        8        1 (    3.47 +/-     1.68)       52        1 (    6.04 +/-     2.75)
 361 nsPrincipal                                    76       76        7        1 (    3.77 +/-     1.96)       76        2 (    8.09 +/-     4.57)
 363 nsProgressNotificationProxy                    32       32        4        1 (    1.43 +/-     0.98)       26        1 (    2.35 +/-     1.05)
 364 nsProperties                                    8        8        4        1 (    2.29 +/-     1.11)       14        1 (    2.96 +/-     1.26)
 397 nsSocketTransportService                     1684     1684        1        1 (    1.00 +/-     0.00)       24        1 (    5.79 +/-     2.22)
 398 nsStandardURL                                 188      752      405        4 (  165.24 +/-    95.23)     3967        8 (  307.50 +/-   172.74)
 402 nsStringBuffer                                  8      416     6134       52 ( 1921.85 +/-   966.17)    15574       56 ( 4776.68 +/-  2494.98)
 436 nsSupportsCStringImpl                          24       24        4        1 (    2.29 +/-     1.11)       16        1 (    2.84 +/-     1.24)
 442 nsTArray_base                                   4       36     4120        9 ( 1244.14 +/-   650.89)        0        0 (    0.00 +/-     0.00)
 459 nsUnicodeNormalizer                            12       12        1        1 (    1.00 +/-     0.00)        4        1 (    1.71 +/-     0.76)
 466 nsVoidArray                                     4       12      643        3 (  109.79 +/-    51.92)        0        0 (    0.00 +/-     0.00)
 467 nsWeakReference                                16      112       69        7 (   34.76 +/-    17.61)      451        7 (  101.26 +/-    57.30)

Comment 1

[Alon Zakai (:azakai)]

Attachment: example image file

Comment 2

[Alon Zakai (:azakai)]

Attachment: patch

Looks like the issue is that an HttpChannelChild and an nsProgressNotificationProxy form a loop. The cycle collection mechanism is not aware of them, so the loop persists forever.

Not sure I know what I'm doing in this patch, but it seems to work locally. Pushed to try.

I guess the reason this happens in Fennec and not Firefox, is that somehow normal HTTP channels don't enter this situation - only remoted IPC ones do. I have no idea why that is, though - the changes I needed to make are in HttpBaseChannel, which is the basis for both remoted and nonremoted ones. Puzzling.

Comment 3

[Alon Zakai (:azakai)]

Comment on attachment 488616 [details] [diff] [review]
patch

Hmm, this prevents the leak but causes a few odd test failures on tinderbox. I'm not sure what I'm doing wrong here, maybe I am misusing the cycle collector API macros somehow?

Example errors:


8584 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/update/test/chrome/test_0011_check_basic.xul | Checking currentPage.pageid equals finished in pageshow - got "errors", expected "finished"

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/tryserver_fedora_test-xpcshell/build/xpcshell/tests/toolkit/mozapps/update/test/unit/test_0030_general.js | test failed (with xpcshell return code: 0), see following log:

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 488616 [details] [diff] [review]
patch

They cycle is basically here (and the other places that look like this)

http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgLoader.cpp#1229

The interface requester owns the channel, and the channel owns the iterface requester.


Could you try removing the mChannel member from nsProgressNotificationProxy, and to get the load group, just use the passed nsIRequest.

Comment 5

[Alon Zakai (:azakai)]

Attachment: Remove mChannel

Great, that fixes the leak and tryserver likes it too.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 490902 [details] [diff] [review]
Remove mChannel


> NS_IMETHODIMP
> nsProgressNotificationProxy::OnStatus(nsIRequest* request,
>                                       nsISupports* ctxt,
>                                       nsresult status,
>                                       const PRUnichar* statusArg) {
>   nsCOMPtr<nsILoadGroup> loadGroup;
>-  mChannel->GetLoadGroup(getter_AddRefs(loadGroup));
>+  mImageRequest->GetLoadGroup(getter_AddRefs(loadGroup));


Why use mImageRequest?  Can you use |request| here?  I am not sure if they are the same or not.



>   nsCOMPtr<nsILoadGroup> loadGroup;
>-  mChannel->GetLoadGroup(getter_AddRefs(loadGroup));
>+  mImageRequest->GetLoadGroup(getter_AddRefs(loadGroup));

same.


I guess I am just not really sure why we have an mImageRequest.   Probably something obvious.   Assuming that you have a good answer, or you can use |request|, r+ from me.  Jduell should also review.

Comment 7

[Alon Zakai (:azakai)]

(In reply to comment #6)
> Comment on attachment 490902 [details] [diff] [review]
> Remove mChannel
> 
> 
> > NS_IMETHODIMP
> > nsProgressNotificationProxy::OnStatus(nsIRequest* request,
> >                                       nsISupports* ctxt,
> >                                       nsresult status,
> >                                       const PRUnichar* statusArg) {
> >   nsCOMPtr<nsILoadGroup> loadGroup;
> >-  mChannel->GetLoadGroup(getter_AddRefs(loadGroup));
> >+  mImageRequest->GetLoadGroup(getter_AddRefs(loadGroup));
> 
> 
> Why use mImageRequest?

For consistency with the other code in that class that uses mImageRequest.

(Perhaps the entire class could be refactored to not have mImageRequest?)

Comment 8

[Jason Duell]

Comment on attachment 490902 [details] [diff] [review]
Remove mChannel

+r with s/mImageRequest->/request->/ 

>-  mChannel->GetLoadGroup(getter_AddRefs(loadGroup));
>+  mImageRequest->GetLoadGroup(getter_AddRefs(loadGroup));
> 
> Why use mImageRequest?  Can you use |request| here?  I am not sure if they are
> the same or not.

I just poked around and debugged some, and the answer is no, request != mImageRequest.  But request == mChannel (even across redirects).  So we can keep the bulk of this patch (i.e. get rid of mChannel), provided we use

    request->GetLoadGroup(getter_AddRefs(loadGroup));

> (Perhaps the entire class could be refactored to not have mImageRequest?)

I don't think so--the mImageRequest seems to be some sort of deliberate "proxy" channel.  I have no idea what that means, though, even after staring at it for a while. (stuart wrote this code--ask him.).  

Anyway, for purposes of this bugfix let's just go with what we've got here, since we know it's correct.

Comment 9

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/5081913b695a