Bug 119312 – Replace numerous manual CRASH's in WebCore with RELEASE_ASSERT

• Status: NEW
• Component: WebCore Misc.

[Kwang Yul Seo]

Manual change from if (foo) CRASH(); to RELEASE_ASSERT(!foo); or if (!foo) CRASH(); to RELEASE_ASSERT(foo);

[Kwang Yul Seo]

Created attachment 207806 [details]
Patch

[Oliver Hunt]

Comment on attachment 207806 [details]
Patch

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

> Source/WebCore/dom/Text.cpp:135
> -        if (std::numeric_limits<unsigned>::max() - data.length() < resultLength)
> -            CRASH();
> +        RELEASE_ASSERT(std::numeric_limits<unsigned>::max() - data.length() >= resultLength);

I'd rather this simply be changed to have resultLength be a Checked<> type

> Source/WebCore/platform/SharedBuffer.cpp:77
> -    if (size < 0)
> -        CRASH();
> +    RELEASE_ASSERT(size >= 0);

Is there a reason that m_size can't be Checked<>?

> Source/WebCore/platform/audio/AudioArray.h:58
> +        RELEASE_ASSERT(n <= std::numeric_limits<unsigned>::max() / sizeof(T));

Again, Checked<> makes quick universally sound work of this :)

[Kwang Yul Seo]

(In reply to comment #2)
> > Source/WebCore/dom/Text.cpp:135
> > -        if (std::numeric_limits<unsigned>::max() - data.length() < resultLength)
> > -            CRASH();
> > +        RELEASE_ASSERT(std::numeric_limits<unsigned>::max() - data.length() >= resultLength);
> 
> I'd rather this simply be changed to have resultLength be a Checked<> type

I filed a separate bug for this. See Bug 119327.
 
> > Source/WebCore/platform/SharedBuffer.cpp:77
> > -    if (size < 0)
> > -        CRASH();
> > +    RELEASE_ASSERT(size >= 0);
> 
> Is there a reason that m_size can't be Checked<>?

The assertion here is about size, not m_size because size is signed int and m_size is unsigned int. As mentioned in the FIXME comment above, some time we should change the type of size to unsigned.

> > Source/WebCore/platform/audio/AudioArray.h:58
> > +        RELEASE_ASSERT(n <= std::numeric_limits<unsigned>::max() / sizeof(T));
> 
> Again, Checked<> makes quick universally sound work of this :)

Also done in Bug 119327.

[Kwang Yul Seo]

Created attachment 207814 [details]
Patch

[Brady Eidson]

Comment on attachment 207814 [details]
Patch

Assuming that patches for review since 2013 are stale, r-