1038897 - setting image cache size to zero fails NULL pointer assertion in imgLoader::CheckCacheLimits

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Georgios Kontaxis]

Attachment: pop entries off the cache to get under the limit provided that the limit is positive

Comment 2

[Georgios Kontaxis]

Comment on attachment 8456394 [details] [diff] [review]
pop entries off the cache to get under the limit provided that the limit is positive

setting preference image.cache.size to <= 0 with an empty queue will still allow a single queue.Pop() iteration which will cause the NULL pointer assertion to fail. The browser will never start. We need to make sure that sCacheMaxSize is meaningful (positive)

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8456394 [details] [diff] [review]
pop entries off the cache to get under the limit provided that the limit is positive

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

::: image/src/imgLoader.cpp
@@ +1305,5 @@
>      NS_ASSERTION(queue.GetSize() == 0,
>                   "imgLoader::CheckCacheLimits -- incorrect cache size");
>  
>    // Remove entries from the cache until we're back under our desired size.
> +  while (queue.GetSize() >= sCacheMaxSize && sCacheMaxSize > 0) {

With this implementation, don't we never pop anything if |sCacheMaxSize == 0|?

I'd suggest a different, two step approach:

(1) Change the while loop here to check |queue.getSize() > sCacheMaxSize|. The current way doesn't make sense anyway - if you set the max size to 10, it pops until the queue size is 9!

(2) To avoid negative numbers, make GlobalInit() clamp sCacheMaxSize so it's always >= 0.

Comment 4

[Georgios Kontaxis]

I thought someone wanted it this way (pop until you reach max size - 1). Will fix.

Comment 5

[Georgios Kontaxis]

Attachment: pop entries off the cache to get under the limit provided that the limit is positive

Comment 6

[Georgios Kontaxis]

Attachment: pop entries off the cache until its size reaches max cache size. Input read from corresponding pref is clamped to non-negative values

Comment 7

[Georgios Kontaxis]

https://tbpl.mozilla.org/?tree=Try&rev=3ba9dce9a686

Comment 8

[Georgios Kontaxis]

Attachment: pop entries off the cache until its size reaches max cache size. Input read from corresponding pref is clamped to non-negative values

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to gkontaxis from comment #4)
> I thought someone wanted it this way (pop until you reach max size - 1).
> Will fix.

It could be that it was intentional, indeed, but I still think it doesn't make much sense. There doesn't seem to be any reason to keep it that way.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8458795 [details] [diff] [review]
pop entries off the cache until its size reaches max cache size. Input read from corresponding pref is clamped to non-negative values

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

Looks good!

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc53a345824f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc53a345824f

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8458795 [details] [diff] [review]
pop entries off the cache until its size reaches max cache size. Input read from corresponding pref is clamped to non-negative values

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

::: image/src/imgLoader.cpp
@@ +1030,5 @@
>    if (NS_SUCCEEDED(rv))
>      sCacheMaxSize = cachesize;
>    else
>      sCacheMaxSize = 5 * 1024 * 1024;
> +  sCacheMaxSize = sCacheMaxSize > 0 ? sCacheMaxSize : 0;

Isn't it already too late?  In the success case above, we assigned int32_t cachesize to uint32_t sCacheMaxSize.  I think the code should have been:

if (NS_SUCCEEDED(rv))
  sCacheMaxSize = cachemaxsize > 0 ? cachemaxsize : 0;
else
  sCacheMaxSize = 5 * 1024 * 1024;

and then there is no need for the sCacheMaxSize = sCacheMaxSize > 0 ? sCacheMaxSize : 0 at all (which really doesn't do anything for uint32_t anyway.)

I'm changing this code in bug 980036, but this is already in 33 Beta, so I'll open another bug for just so that we can discuss if we need an uplift.