Bug 114356

Summary: REGRESSION(r148034): IconDatabase::updatIconRecord crashes with null iconData and iconBitmap.
Product: WebKit Reporter: Raphael Kubo da Costa (:rakuco) <rakuco>
Component: WebCore Misc.Assignee: Raphael Kubo da Costa (:rakuco) <rakuco>
Status: RESOLVED FIXED    
Severity: Normal CC: oliver, simon.fraser, spenap, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch thorton: review+

Description Raphael Kubo da Costa (:rakuco) 2013-04-10 06:53:59 PDT 
REGRESSION(r148034): IconDatabase::updatIconRecord crashes with null iconData and iconBitmap.
Comment 1 Raphael Kubo da Costa (:rakuco) 2013-04-10 06:57:37 PDT 
Created attachment 197262 [details]
Patch
Comment 2 Tim Horton 2013-04-10 08:45:53 PDT 
Comment on attachment 197262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197262&action=review

> Source/WebCore/loader/icon/IconDatabase.cpp:541
> +    ASSERT(!(!!iconData && !!iconBitmap));

!! is a bit bizarre but there are other instances of it around. But I wonder why && alone isn't enough?
Comment 3 Raphael Kubo da Costa (:rakuco) 2013-04-10 09:05:55 PDT 
(In reply to comment #2)
> (From update of attachment 197262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197262&action=review
> 
> > Source/WebCore/loader/icon/IconDatabase.cpp:541
> > +    ASSERT(!(!!iconData && !!iconBitmap));
> 
> !! is a bit bizarre but there are other instances of it around. But I wonder why && alone isn't enough?

I was just being extra-cautious here; PassRefPtr::operator!() was casting them into bools, and I'm adding an extra ! to get the "original" boolean value.
Comment 4 Raphael Kubo da Costa (:rakuco) 2013-04-10 09:12:21 PDT 
Committed r148097: <http://trac.webkit.org/changeset/148097>
Comment 5 Darin Adler 2013-04-10 09:42:32 PDT 
Comment on attachment 197262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197262&action=review

>>> Source/WebCore/loader/icon/IconDatabase.cpp:541
>>> +    ASSERT(!(!!iconData && !!iconBitmap));
>> 
>> !! is a bit bizarre but there are other instances of it around. But I wonder why && alone isn't enough?
> 
> I was just being extra-cautious here; PassRefPtr::operator!() was casting them into bools, and I'm adding an extra ! to get the "original" boolean value.

No, those !! are not needed unless there is a bug in PassRefPtr. The && operator should work fine with two PassRefPtr values without requiring the ! at all.

It would be nice to remove those.
Comment 6 Raphael Kubo da Costa (:rakuco) 2013-04-11 02:16:28 PDT 
(In reply to comment #5)
> (From update of attachment 197262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197262&action=review
> 
> >>> Source/WebCore/loader/icon/IconDatabase.cpp:541
> >>> +    ASSERT(!(!!iconData && !!iconBitmap));
> >> 
> >> !! is a bit bizarre but there are other instances of it around. But I wonder why && alone isn't enough?
> > 
> > I was just being extra-cautious here; PassRefPtr::operator!() was casting them into bools, and I'm adding an extra ! to get the "original" boolean value.
> 
> No, those !! are not needed unless there is a bug in PassRefPtr. The && operator should work fine with two PassRefPtr values without requiring the ! at all.
> 
> It would be nice to remove those.

https://bugs.webkit.org/show_bug.cgi?id=114425