[webkit-reviews] review granted: [Bug 196855] Fix Covscan uninitialized after ctor : [Attachment 367314] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Eike Rathke
<erack at redhat.com>'s request for review:
Bug 196855: Fix Covscan uninitialized after ctor
https://bugs.webkit.org/show_bug.cgi?id=196855

Attachment 367314: Patch

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




--- 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.