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

Sat Apr 13 09:23:05 PDT 2019

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

--- Comment #24 from Darin Adler <darin at apple.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

I’m OK with making these changes.

Many are correctness problems, but many are not correctness problems.

I know that some on the project are certain we should make almost all these changes because they are either helpful or harmless. I believe that is overstating the case because in the past additional initialization has had measurable performance costs. I hope that none of these changes are in that category, but it would be overconfident to say that we are certain there are none.

We could also just say we want to take these changes so we can continue to use Coverity to find future problems and quieting the warnings is worth it.

I audited most of these to the best of my ability and added comments to many that are not correctness problems; I can provide more detail if someone is interested in why, but I think it‘s obvious from reading over the code in most cases.

Probably OK to make all of these changes, despite the many that are not needed.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1334
> +    Node* m_currentNode { nullptr };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or safety decision rather than a correctness decision.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14203
> +        StringImpl* string { nullptr };
> +        LBasicBlock target { nullptr };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/JavaScriptCore/parser/Nodes.h:2042
> +        FunctionMode m_functionMode { FunctionMode::FunctionExpression };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/JavaScriptCore/runtime/ConfigFile.cpp:236
> +    FILE* m_file { nullptr };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/JavaScriptCore/runtime/ErrorInstance.h:101
> +    unsigned m_line { 0 };
> +    unsigned m_column { 0 };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/JavaScriptCore/runtime/JSBigInt.h:246
> +    unsigned m_length { 0 };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

And I don’t understand which constructor Coverity is warning about here. I only see one constructor and it does initialize m_length.

> Source/JavaScriptCore/runtime/PropertySlot.h:390
> +    unsigned m_attributes { 0 };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

This is a peculiar case. The PropertySlot constructor does not initialize one of the most important members, m_data, but Coverity does not warn about it presumably because it’s complex. Initializing just this one more data member has negligible value. Typically PropertySlot objects are initialized by the various set functions, all of which set m_attributes at the same time that they set m_data.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:61
> +    double m_playbackTime { 0.0 };

Not sure that 0 is the correct default value here. Would be good to have a test covering this.

> Source/WebCore/accessibility/AccessibilityTableColumn.h:63
> +    unsigned m_columnIndex { 0 };

Not clear to me that a 0 is an acceptable default here. I suppose initialized is better than not, but 0 can be initialized but incorrect.

> Source/WebCore/animation/DeclarativeAnimation.h:95
> +    double m_previousIteration { 0.0 };

I think NAN is a better default here than 0. Not sure.

> Source/WebCore/dom/RequestAnimationFrameCallback.h:46
> +    int m_id { 0 };
> +    bool m_firedOrCancelled { false };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:95
> +    ShBuiltInResources m_resources { };

Seems like a mistake to initialize this with aggregate initialization instead of by calling the InitBuiltInResources function. Not sure if this is needed.

> Source/WebCore/platform/graphics/GradientImage.h:57
> +    unsigned m_cachedGeneratorHash { 0 };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:224
> +    bool m_hasAlphaChannel { false };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.h:83
> +    GLint m_internalFormat { 0 };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

>> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:59
>> +    , 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.

That’s right, has to be done this way because they are bitfields.

> Source/WebCore/platform/text/TextCodecUTF16.h:45
> +    unsigned char m_bufferedByte { 0 };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/WebCore/rendering/RenderFragmentedFlow.h:230
> +        bool m_rangeInvalidated { false };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/WebCore/workers/WorkerScriptLoader.h:97
> +    FetchOptions::Destination m_destination { FetchOptions::Destination::EmptyString };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

> Source/WebKit/Shared/Plugins/PluginProcessCreationParameters.h:50
> +    PluginProcessType processType { PluginProcessTypeNormal };

Not needed; as far as I can tell this is a coding style and "make the static checker happy" or future-proofing/safety measure rather than a correctness decision.

-- 
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/20190413/fcaf99e2/attachment-0001.html>