Bug 149011

Summary: Fix null dereferences from static analysis
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: WebKit Misc.Assignee: Alex Christensen <achristensen>
Status: NEW ---    
Severity: Normal CC: ahmad.saleem792
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review+

Description Alex Christensen 2015-09-09 15:03:10 PDT 
I guess this will make things better.
Comment 1 Alex Christensen 2015-09-09 15:05:49 PDT 
Created attachment 260876 [details]
Patch
Comment 2 Alex Christensen 2015-09-09 15:22:04 PDT 
Created attachment 260881 [details]
Patch
Comment 3 Darin Adler 2015-09-09 16:42:37 PDT 
Comment on attachment 260881 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Fix null dereferences from static analysis

You mean “fix null dereferences detected by static analysis tool”?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:188
> -    RELEASE_ASSERT(record->patchpointID == exit.m_stackmapID);
> +    RELEASE_ASSERT(record && record->patchpointID == exit.m_stackmapID);

In a normal assert we would have done two separate ones, so we could tell of the two && was failing. Not sure if that thinking applies to RELEASE_ASSERT.

> Source/WTF/wtf/ParkingLot.cpp:330
> +    RELEASE_ASSERT(oldHashtable && newSize > oldHashtable->size);

Same comment about && in assertions.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:623
> -    if (isRemoved())
> +    if (!m_source)
>          return;

Isn’t the static analyzer smart enough to understand that this is the same thing?

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:273
> -            ASSERT_WITH_MESSAGE(sourceActiveSubtree, "The root should always have a valid generator.");
> +            RELEASE_ASSERT_WITH_MESSAGE(sourceActiveSubtree, "The root should always have a valid generator.");

This doesn’t seem like a good change to me. How is adding a null check at runtime helpful to the project?

> Source/WebCore/css/SelectorCheckerTestFunctions.h:-185
> -        unsigned rangeSubtagsEndIndex = rangeLength;

This is not a fix to a null dereference.

> Source/WebCore/rendering/CounterNode.cpp:84
> +                    if (nextSibling)
> +                        nextSibling->m_previousSibling = child;

I do think this is a real bug. Can we figure out how to make a test case for this?
Comment 4 Alexey Proskuryakov 2015-09-09 19:12:05 PDT 
I think that it's important to tell which part of release assert fails. Otherwise, investigating frequent crashes would be unnecessarily harder.