[Webkit-unassigned] [Bug 196855] Fix Covscan uninitialized after ctor

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at igalia.com
 Attachment #367314|review?, commit-queue?      |review+, commit-queue-
              Flags|                            |

--- Comment #7 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 367314
  --> https://bugs.webkit.org/attachment.cgi?id=367314
Patch

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

Although I agree with Darin, I'm also generally of the opinion that we should change WebKit to please as many static analysis tools as we can. Even when the tool reports false positives, as long as it's not too many, then avoiding those will make it easier to see when there are real problems. This is my approach to GCC compiler warnings, and I believe it has worked well there. In one previous discussion, we were perhaps too strict about whether we should fix UNINIT_CTOR warnings, even proposing in one case that some should not be fixed so that we could diagnose with asan if code outside the constructor failed to initialize particular members. While that could work in theory, I don't think it was the right judgment call, and work on fixing Coverity issues stalled as a result, which was not good for WebKit. I like the rule that constructors should initialize all data members, and our code should be more robust and contain fewer bugs if we follow it.

Anyway, looking through this patch, the changes mostly look good. I'm impressed that we have fewer issues here than I would have expected.

We have several existing Coverity bugs: bug #104114, bug #186798, bug #190468. We should be aware of those as well, especially the metabug #104114. I would remove the HTMLMediaElement.cpp change from this patch, since it's clearly not a UNINIT_CTOR issue, and fix that in a new bug marked against bug #104114 instead. Also, since there are a relatively large number of whitespace fixes in AccessibilityTableColumn.h, those should be landed separately as well; if it was a smaller amount, I wouldn't consider it worth a separate patch, but that's enough to obscure the fix you've made there.

BTW, welcome Eike. :) Darin, Eike is the new maintainer of Red Hat's WebKit and Chromium packages.

> Source/JavaScriptCore/dfg/DFGOSRExit.h:135
> -    ExtraInitializationLevel extraInitializationLevel;
> +    ExtraInitializationLevel extraInitializationLevel { };

You could choose to be explicit here: extraInitializationLevel { ExtraInitializationLevel::None };

But I think this is fine, too.

> Source/JavaScriptCore/runtime/CodeCache.cpp:141
> -        JSToken token;
> +        JSToken token = { };

I think you should fix this one in ParserTokens.h:

struct JSToken {
    JSToken() = default;
    // ...
};

to match the other tokens defined in that file. That would be much more robust.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:154
> +        ResultList result { };

But ResultList is:

Vector<Value*, 1>

so this one shouldn't need the brace initialization... correct?

> Source/WebCore/Modules/webaudio/DelayDSPKernel.h:53
>      int m_writeIndex;

Both constructors initialize this to 0, so might as well modernize them by initializing it here instead of in the constructors' initializer lists.

> Source/WebCore/Modules/webaudio/DelayDSPKernel.h:56
>      bool m_firstTime;

Ditto.

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:59
> +    : m_hasForwardButtonStartPart(false)
> +    , m_hasForwardButtonEndPart(false)
> +    , m_hasBackButtonStartPart(false)
> +    , m_hasBackButtonEndPart(false)

Hm, is it possible to initialize these in the class declaration instead? Or is that not possible, because these are bitfields? If so, this is fine.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190412/ab8b6651/attachment-0001.html>