Closed Bug 1038897 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: geko1702+bugzilla, Assigned: geko1702+bugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → gkontaxis
Status: NEW → ASSIGNED
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)
Attachment #8456394 - Flags: review?(seth)
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.
Attachment #8456394 - Flags: review?(seth) → review-
I thought someone wanted it this way (pop until you reach max size - 1). Will fix.
Attachment #8456394 - Attachment is obsolete: true
Attachment #8458781 - Attachment is obsolete: true
Attachment #8458789 - Attachment is obsolete: true
Attachment #8458795 - Flags: review?(seth)
(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 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!
Attachment #8458795 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/fc53a345824f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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.
You need to log in before you can comment on or make changes to this bug.