295074 - POST responses remain in the memory cache when using XMLHttpRequest => huge memory leak

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Sascha Schumann]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050522 Firefox/1.0+

When XMLHttpRequest sends a POST request, the complete response remains in the
memory cache. 

This becomes a huge issue in XUL applications which often generate thousands of
POST requests per day.

Also closing a page does not wipe out the relevant memory cache entries.

Mozilla also ignores anti-caching headers (I am not sure whether they should
affect the memory cache).

Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Pragma: no-cache

The behaviour can be observed in XMLHttpRequest's sync and async mode.

Compressing/gzipping the output reduces the impact of this issue, as the memory
cache apparently contains the literal data sent by the server. 

Reproducible: Always

Steps to Reproduce:
0. Start a fresh Mozilla
1. Go to http://chatserv.de/xmlhttprequest.html
2. Wait until the counter reaches 299. An alert will pop up.
3. Open "about:cache"

Actual Results:  
Memory usage increases by at least 300 * 17KB = 5MB.

Expected Results:  
POST responses are NOT cachable, especially if the server tells you not to cache. 

Mozilla must not cache the responses or at least free them as early as possible.

Comment 1

[Darin Fisher]

Have you looked at "about:cache" when this happens?  Is the memory cache
exceeding its size limits?  This may be working as designed.

Comment 2

[Sascha Schumann]

The memory cache is not exceeding its size limits.

My main point:

XMLHttpRequest should not pollute the memory cache with uncachable responses.

Christian Biesinger mentioned that caching _all_ responses might be necessary
for view source/save page.

That is certainly true in the browser context. It does not explain though why
XMLHttpRequest should waste resources with responses who will never ever be used
again. I hope that is not by design.

Comment 3

[Darin Fisher]

> That is certainly true in the browser context. It does not explain though why
> XMLHttpRequest should waste resources with responses who will never ever be used
> again. I hope that is not by design.

Agreed.  I don't believe the authors of nsXMLHttpRequest intended this behavior.
 I suspect they were unaware of the issue.

My comments (about design) were in regard to the default caching behavior of the
networking library on which nsXMLHttpRequest is implemented.

A fix is probably trivial to implement.  We should simply add the
INHIBIT_CACHING load flag to the channel when issuing a POST request via
nsXMLHttpRequest.

Comment 4

[Darin Fisher]

Attachment: v1 patch

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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

(if anyone wants to consider this for any branches, they should note bug 289374
(and bug 232385))

Comment 6

[Sascha Schumann]

v1 patch tested + confirmed: There are no responses in either cache from
XMLHttpRequest. Memory usage remains stable throughout the test run of
http://chatserv.de/xmlhttprequest.html .

Comment 7

[Johnny Stenback (:jst)]

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

sr=jst

Comment 8

[Boris Zbarsky [:bzbarsky]]

Nominating for blocking so we don't forget to land this ready-to-go patch.

Comment 9

[Darin Fisher]

fixed-on-trunk

Comment 10

[justin hugh daly]

is this supposed to be fixed in the firefox beta?

when i launch the test url: http://chatserv.de/xmlhttprequest.html

and view the stats on about:cache and about:cache?device=memory i still see
memory increases and entries for chatserv

can anyone confirm this?

Comment 11

[Darin Fisher]

Interesting.  I see that problem too.  It seems to have avoided the disk cache,
but it still populates the memory cache.  That's definitely not what was desired.

Comment 12

[Darin Fisher]

reopening.  this isn't fixed.

Comment 13

[Darin Fisher]

What's odd about this bug is that I cannot reproduce the problem using a debug
build.  In my debug build, the memory cache entries do not exist, but in my
release build, I do see the objects in the memory cache.  This is very strange.

Comment 14

[Scott MacGregor]

Sounds like Darin is asctively looking into this and the leak sounds bad to me.
But we'll re-evaluate the nomination once we have a fix to do risk analysis on.

Comment 15

[Darin Fisher]

WOW.  Ok, I don't know how to explain this yet, but it appears that I can
reproduce this bug if I manually set browser.cache.memory.capacity to some fixed
value.  If I set it to -1 or if I remove the pref (which is the default
configuration), then this bug does not occur.  That pref only controls the size
of the memory cache, it does not disable the memory cache, so I don't know why
it would impact this bug.  Investigating...

Comment 16

[Darin Fisher]

OK.  More info:  It turns out that if you have more than 2GB of RAM on your
system (Linux), then your memory cache size will be zero!  This is due to bug
308587.  If your memory cache size is zero, then this bug will appear to be fixed.

This explains why I observed an effect by setting browser.cache.memory.capacity
manually in my prefs.

So, the net take-away is that this bug is indeed still here.  So, I will try to
figure out why the cache entries are not being cleared.

Comment 17

[Darin Fisher]

OK, the problem observed with the testcase is caused by the fact that null is
passed to .send().  As a result, we do not have a postDataStream, and therefore
we do not run through the code that calls SetUploadStream.  I think we need to
trigger the loadFlag munging code when the method is POST and not just when the
postDataStream is non-null.

Comment 18

[Darin Fisher]

Attachment: v2 patch

This patch does the trick.  The first set of changes just eliminates a compile
warning under GCC 3.2.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 196717 [details] [diff] [review]
v2 patch

r=biesi

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 196717 [details] [diff] [review]
v2 patch

sr=jst

Comment 21

[Darin Fisher]

fixed-on-trunk

Comment 22

[Tracy Walker [:tracy]]

I can verify this on Firefox trunk nightly builds from 09-23 Wndows and Linux. 
No response.txt is getting cached in memory or disc. But what is puzzling me is
that Mac 1.5 branch build from 9-23 also didn't show any response.txt in cache.
I verified I am correctly understanding the testcase with a test on Moz 1.7.12.
Note that I did not see the status count to 300 on the Firefox builds as it did
on Moz 1.7.12.  The only 2 Firefox cache entries are for an empty favicon and
the html page itself.

Please verify.

Comment 23

[Darin Fisher]

fixed1.8

Comment 24

[Worcester12345]

mlk keyword missing (for historical purposes)

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

er, this is not actually a leak, is it?

Comment 26

[Darin Fisher]

(In reply to comment #25)
> er, this is not actually a leak, is it?

Technically, yeah.. you are correct.