47660 - put a content length limit on image going into image cache.

Status: VERIFIED INVALID

(Core :: Graphics: ImageLib, defect, P2)

Description

[pnunn]

put a content length limit on image going into image cache.
-p

Comment 1

[pnunn]

*** Bug 20939 has been marked as a duplicate of this bug. ***

Comment 2

[pnunn]

getting code reviews.

Comment 3

[Gagan]

this is important to avoid major problems. And so a P2. 

Comment 4

[selmer (gone)]

pdt agrees p2.

Is Eli still the correct QA contact for this?

Comment 5

[pnunn]

I'd be happy to check this in, but the super reviewers haven't
responded to my email.
-p

Comment 6

[Peter Trudelle]

PDT marking nsbeta3-, nominating for rtm

Comment 7

[Gagan]

must go in for rtm.

Comment 8

[Michael La Guardia]

Marking "needinfo". This seems like a bad thing, but we need a patch, review and
super review to clear for checkin.  Could you add more info of the horrible
things that will happen if this isn't fixed?  Otherwise, as we get closer to the
15th this will be a candidate for minusing.

Comment 9

[pnunn]

I have a fix. reviewers@moz didn't like the fix because
they didn't feel content-length was adequate to test large
images even though it is the only info available before the
image file header has been read. The reviewer felt that the
fact we were testing against compressed data length rather than
decompressed data length made the patch unacceptable.

I tried to explain that the check was intended to catch very 
large images as early as possible. I'll attach the diffs to this bug,
in case someone else wants to push for this checkin. I have other
bugs to address.

-pn

Comment 10

[pnunn]

Attachment: diffs from mozilla/modules/libimg

Comment 11

[pnunn]

Attachment: diffs from mozilla/gfx

Comment 12

[tor]

Here's an attempt to use the actual image size by throwing images out of the
cache once we know their true size in il_size().  It seems to work (based on
about ten minutes of browsing).  The only problem I see with this is that
items are being pushed out of the cache to insert an image which might later
be thrown out.  Are there any other things to consider?

Experimentation quality patch:

Index: if.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/libimg/src/if.cpp,v
retrieving revision 3.63
diff -u -r3.63 if.cpp
--- if.cpp      2000/09/13 23:53:54     3.63
+++ if.cpp      2000/10/06 20:50:54
@@ -827,6 +827,12 @@
         ic->dest_height = req_h;
     }
 
+    if (image_bytes>65536) {
+      fprintf(stderr, "**** EJECTING from cache (%s) [%d]\n", 
+              ic->url_address, image_bytes);
+      il_removefromcache(ic);
+    }
+
     return 0;
 }

Comment 13

[pnunn]

The content length test would help keep
really large images from ever getting into the imgcache....
which is why we want this test.
-pn

Comment 14

[tor]

A content length test should be left for necko's memory cache, which
doen't have any additional information.  libimg's image cache knows
more and should take advantage of that.  My patch is a minimal
impact attempt at doing this.

Comment 15

[pnunn]

I disagree.

Comment 16

[tor]

The latest set of patches turns on PIN_CHROME, which wasn't in
the earlier patches and was turned off (ilclient.cpp 3.33) to
prevent disappearing chrome images.  Your patch also wipes out
bienvenu's change (ilclient.cpp 3.34) to prevent a dereference of
a null ic.  Except for these two things, a=tor for the branch,
since that's under time pressure.

Please leave the bug open for a mozilla resolution, which we can
spend some more time on and hopefully find a solution which
satisfies everyone.

Comment 17

[pnunn]

PDT:
I've got a super review. I've made the changes described
above. I've retested with a fresh tree.

Can I get a ++ from you? or what ever it is I need to check into
branch....

-pn

Comment 18

[pnunn]

Removed the "need info" keywork on the Status Whiteboard hopefully
indicating this check in is ready to go.
-pn

Comment 19

[Phil Peterson]

We'd still like to know the benefit to taking this (or cost of not taking it) as
Michael asked. Patches going into the branch now need to have very strong
benefits to end-users, as well as low risk. Back to [need info]

Comment 20

[Gagan]

this is very low risk. The worst effect it will have is that huge images beyond 
certain content length will have to be refetched from the server. 

The benefit is that we'd ship with a much healthier imagelib cache that 
continues to be usable for more than a few pages. 

The risk of not taking this is that we'd be misusing our imagelib cache to fill 
up with huge images and make the imagelib cache unusable after surfing for a 
while. 

Setting this back to rtm+ and strongly recommending rtm++

Comment 21

[pnunn]

The changes is a drop in a bucket of changes that help conserve resources.
In particular, there has been some concern about the imgcache hogging
more memory than it needs to hog.

I have set a 2M upper limit on the imgcache, but this can be exceeded by
one huge image coming in. Eventually the one huge image will be removed
from the cache.

This change allows us to prevent the addition huge images into the imgcache
as soon as possible. The discussion between tor & I had to do with differences
of opinion of implementation. This patch checks the content length (which is
part of the incoming channel info)of the compressed image data. When
decompressed, the image data size will be larger. My point is 
that if we already know the content length is huge, we shouldn't even try to 
add the image to the imgcache. tor preferred the idea of removing the image
from the image cache after the target dimensions are known so you can test on 
the decompressed size.

I think both are valid and should be used. 
(And later, after all this hoooha, I'd like to move the addition of the 
image into the imgcache until later in the process.)

And this bug only convers the patch that uses content length for the test.
So this is the only piece of the fix I'm proposing should be checked in.


-P
 

Comment 22

[pnunn]

Attaching a set of 2 patches of final fix.
I noticed that at shutdown the imgcache had its
total bytes adjusted by the amount of the huge
image, even though the huge image is not placed in the
imgcache. At shutdown the imgcache size should be
0. It was not.

The imgcache size is adjusted not only at the time of
adding an image to the imgcache, but alao when the image
is sized. I added an additional check so the total byte
count of the imgcache is not adjusted when the image is sized.

-p

Comment 23

[pnunn]

Attachment: diffs from gfx directory

Comment 24

[pnunn]

Attachment: diffs from libimg directory

Comment 25

[selmer (gone)]

we're very afraid of the amount of changes in your libimg patch.  Could you show 
us a version that is smaller and specifically addresses this problem only?  
(Please do not include any additional cleanup or other unrelated changes)

Comment 26

[pnunn]

The only clean up is done in the gfx diffs and the cleanup
consists of removing a defunct method of the same name...which
might cause some confusion.

The libimg changes may look like it contains spurious clean up changes,
but code is moved from one location to another, because a change in
sequence is needed. 

-p

Comment 27

[Jason Eager]

If the PDT isn't going to take this, then it should be marked rtm- to take it
off the rtm radar. Otherwise, we need a r= and a sr= asap.

Comment 28

[Gagan]

pnunn not comfortable with these changes. Too late for rtm to be going back to
the design board. rtm- per PDT

Comment 29

[pnunn]

This issue will be addressed in the new cache design.
This bug no longer applies.
-pn

Comment 30

[Terri Preston]

Per pnunn's comments, marking verified invalid