Bug 89796 – Address uninitialized member variable issues to make Coverity happier

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[kmadhusu]

Address uninitialized member variable issues to make Coverity happier.

[kmadhusu]

Created attachment 149144 [details]
Patch

[James Robinson]

Comment on attachment 149144 [details]
Patch

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

Do we have any sort of plan for keeping this fixed instead of requiring patches all the time?

> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:160
> +        };

no trailing ; please

[kmadhusu]

Created attachment 149145 [details]
Patch

[kmadhusu]

James: Addressed review comments. PTAL. 

Generally, I try to send the patch to the corresponding owners, so that they are aware of these issues and does not repeat them in the future. Reviewers should watch for these types of issues while reviewing code. Ideally, Coverity tool manager should notify the owners of the code about the issues. If possible, we should have a tool that can catch these errors while uploading the CL for review.

Thanks.

[James Robinson]

Without tooling, this doesn't feel sustainable.

[Alexey Proskuryakov]

It feels that the effort needed to keep Coverity happy, as well as the effects on code readability and performance, much outweigh the benefits. I can only remember a few minor mistakes found by it.

[Kent Tamura]

Comment on attachment 149145 [details]
Patch

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

> Source/WebCore/html/HTMLFieldSetElement.cpp:41
> +    , m_documentVersion(0)

OK for this part.  This is an actual bug fix.

> Source/WebKit/chromium/src/WebPagePopupImpl.cpp:145
> +    , m_webView(0)
> +    , m_popupClient(0)

This initialization is not needed.  But it's ok to initialize them with 0.

[James Robinson]

Comment on attachment 149145 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.h:78
> +            , format(0)

this should probably default to GraphicsContext3D::INVALID_VALUE

[kmadhusu]

Comment on attachment 149145 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.h:78
>> +            , format(0)
> 
> this should probably default to GraphicsContext3D::INVALID_VALUE

Fixed.

[kmadhusu]

+ Adding more owners to review the individual files.

barraclough  => YarrInterpreter.cpp
bdakin       => CSSImageSetValue.h
kling        => StylePropertySet.cpp, DynamicNodeList.h
ddorwin      => MediaKeyEvent.cpp
gman         => WebGLFramebuffer.cpp  
vsevik       => NetworkResourcesData.cpp
igor.o       => CSSPropertyAnimation.cpp
wjmaclean    => TouchpadFlingPlatformGestureCurve.cpp
danakj       => CCVideoLayerImpl.h
jpfau        => MarkupTokenBase.h

Please review and let me know your comments.

Thanks.

[Dana Jansens]

CCVideoLayerImpl: It's fine, though unneeded.

[Kent Tamura]

I recommend split the patch into multiple patches.  It's hard for every reviewer to set r+.

[kmadhusu]

(In reply to comment #12)
> I recommend split the patch into multiple patches.  It's hard for every reviewer to set r+.

If I want to commit as multiple patches, I need to file multiple bugs right?

Will it work if the owners specify r+ in the comment and once I get all the comments, one of you can actually set the review:+ flag and add the patch to commit queue?

[James Robinson]

I think what people are saying is that this is a lot of hassle for pretty much no benefit, unless you know of actual bugs that some of these missing initializations are causing.  If you do, then please file a bug on that specific initialization with a fix for that and (ideally) a test case.  Otherwise, this doesn't seem worth spending time on.

[David Dorwin]

Comment on attachment 149145 [details]
Patch

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

> Source/WebCore/html/MediaKeyEvent.cpp:43
> +    : m_systemCode(0)

This looks fine to me. 
There is some interaction with MediaKeyEventInit that may make this unnecessary, but I'd have to look. I thought there were test cases that would have caught this value not being initialized correctly.

[kmadhusu]

I understand your concern. I don't have any actual bugs for the current patch. In future, I will make sure that my coverity patches fixes explicit bugs. I don't know webkit internals well enough to distinguish the important fixes in my patch. Since the reviewers have raised objections to commit this patch, I am closing this bug. 

Thanks.

[Kent Tamura]

Comment on attachment 149145 [details]
Patch

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

>> Source/WebCore/html/HTMLFieldSetElement.cpp:41
>> +    , m_documentVersion(0)
> 
> OK for this part.  This is an actual bug fix.

I have filed https://bugs.webkit.org/show_bug.cgi?id=90038 for this bug.