Bug 119312 - Replace numerous manual CRASH's in WebCore with RELEASE_ASSERT
Summary: Replace numerous manual CRASH's in WebCore with RELEASE_ASSERT
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-30 21:08 PDT by Kwang Yul Seo
Modified: 2016-05-24 22:02 PDT (History)
10 users (show)

See Also:

Attachments
Patch (12.12 KB, patch)
2013-07-30 21:18 PDT, Kwang Yul Seo 		no flags Details | Formatted Diff | Diff
Patch (10.73 KB, patch)
2013-07-31 00:49 PDT, Kwang Yul Seo 		beidson: review-
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2013-07-30 21:08:28 PDT 
Manual change from if (foo) CRASH(); to RELEASE_ASSERT(!foo); or if (!foo) CRASH(); to RELEASE_ASSERT(foo);
Comment 1 Kwang Yul Seo 2013-07-30 21:18:10 PDT 
Created attachment 207806 [details]
Patch
Comment 2 Oliver Hunt 2013-07-30 23:17:31 PDT 
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 :)
Comment 3 Kwang Yul Seo 2013-07-31 00:42:45 PDT 
(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.
Comment 4 Kwang Yul Seo 2013-07-31 00:49:56 PDT 
Created attachment 207814 [details]
Patch
Comment 5 Brady Eidson 2016-05-24 22:02:05 PDT 
Comment on attachment 207814 [details]
Patch

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