Bug 196855 – Fix Covscan uninitialized after ctor

• Status: NEW
• Component: WebKitGTK

[Eike Rathke]

Fix Covscan uninitialized after ctor

[Eike Rathke]

Created attachment 367313 [details]
Patch

[EWS Watchlist]

Attachment 367313 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eike Rathke]

Created attachment 367314 [details]
Patch

[Darin Adler]

Comment on attachment 367314 [details]
Patch

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

I really appreciate the time an effort to run Coverity and fix everything it finds.

This patch includes fixes for some real bugs, and some cases where Coverity effectively just prefers a different coding style. The Coverity coding style may be safer, but we don’t necessarily want to make that kind of a change without a discussion first.

Generally we would not land a patch, especially from a non-contributor, without discussion.

I started looking at the changes in the patch to check them for correctness and value but didn’t have a chance to review the whole thing, just started looking at a couple.

> Source/WebCore/html/HTMLMediaElement.cpp:2105
> -        if (actionIfInvalid == Complain)
> +        if (actionIfInvalid == Complain) {
>              FrameLoader::reportLocalLoadFailed(frame.get(), url.stringCenterEllipsizedToLength());
>              ERROR_LOG(LOGIDENTIFIER, url , " was rejected by SecurityOrigin");
> +        }
>          return false;

Good fix, but a different Coverity error, not uninitialized data members.

> Source/WebCore/html/HTMLMenuElement.h:43
> -    bool m_isTouchBarMenu;
> +    bool m_isTouchBarMenu { false };

Definitely a real bug here.

[Eike Rathke]

(In reply to Darin Adler from comment #4)
> This patch includes fixes for some real bugs, and some cases where Coverity
> effectively just prefers a different coding style. The Coverity coding style
> may be safer, but we don’t necessarily want to make that kind of a change
> without a discussion first.

There may be some false positives now because the actual patches stem from covscans of between 2.20.x to 2.22.7 backported to master.

However, I think there is nothing coding style involved but rather most are indeed some "left uninitialized" errors. Or could you point out an example that you think is only coding style?

> Generally we would not land a patch, especially from a non-contributor,
> without discussion.

Fine with me.

> > Source/WebCore/html/HTMLMediaElement.cpp:2105
> > -        if (actionIfInvalid == Complain)
> > +        if (actionIfInvalid == Complain) {
> >              FrameLoader::reportLocalLoadFailed(frame.get(), url.stringCenterEllipsizedToLength());
> >              ERROR_LOG(LOGIDENTIFIER, url , " was rejected by SecurityOrigin");
> > +        }
> >          return false;
> 
> Good fix, but a different Coverity error, not uninitialized data members.

Historically it has been in a general covscan fixes patch that later was renamed to uninit_ctor probably because by then it contained mostly such fixes..  Certainly the covscan error here indeed was something different.

[Darin Adler]

(In reply to Eike Rathke from comment #5)
> However, I think there is nothing coding style involved but rather most are
> indeed some "left uninitialized" errors. Or could you point out an example
> that you think is only coding style?

I will once I review more. I am pretty sure I spotted some that are technically uninitialized, but semantically there is no need to initialize. A source code analyzer can’t see enough of the semantics to understand the the optimization, lack of initialization, is safe.

[Michael Catanzaro]

Comment on attachment 367314 [details]
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.

[Michael Catanzaro]

I see there's some overlap between this patch and bug #186798, for example, but only some. What's the status of that one? Is the other patch obsolete, or do they need to be combined to comprehensively fix these warnings?

[Michael Catanzaro]

Comment on attachment 367314 [details]
Patch

(r+ -> r? because I see Darin wants to review this further.)

[Michael Catanzaro]

Comment on attachment 367314 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:151
> +        BlockType blockType { };

I originally left a comment complaining that there was no sane default value to use here, but then removed it, deciding the initial value should be good enough. But I notice Mark has left a similar complaint about an equivalent change in bug #186798. This probably requires further discussion with the JSC developers, perhaps in that bug, to decide how to fix this specific instance, as well as your two changes in WasmValidate.cpp. Looks like there is considerable overlap between the fixes in these bugs, at any rate.

[Alexey Proskuryakov]

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

It may be obvious to everyone looking at this bug, but adding initialization where it is not needed regresses performance, almost by definition. Other changes needed to please code analysis tools can be introducing other problems, so I think that each one needs to be discussed on its merits.

[Filip Pizlo]

Comment on attachment 367314 [details]
Patch

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

This patch is almost good.  Please revert whitespace changes.  Please consider applying some of the other feedback.

>> Source/JavaScriptCore/dfg/DFGOSRExit.h:135
>> +    ExtraInitializationLevel extraInitializationLevel { };
> 
> You could choose to be explicit here: extraInitializationLevel { ExtraInitializationLevel::None };
> 
> But I think this is fine, too.

Yeah that would be better in this case.

>> Source/JavaScriptCore/runtime/CodeCache.cpp:141
>> +        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.

Agreed.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:154
>> +        ResultList result { };
> 
> But ResultList is:
> 
> Vector<Value*, 1>
> 
> so this one shouldn't need the brace initialization... correct?

I think that's right.   It's not necessary to have { } on result but it doesn't hurt either.

> Source/WebCore/accessibility/AccessibilityTableColumn.h:60
>  #include "IntRect.h"
>  
>  namespace WebCore {
> -    
> +
>  class RenderTableSection;
>  
>  class AccessibilityTableColumn final : public AccessibilityMockObject {
>  public:
>      static Ref<AccessibilityTableColumn> create();
>      virtual ~AccessibilityTableColumn();
> -    
> +
>      AccessibilityObject* headerObject();
> -        
> +
>      AccessibilityRole roleValue() const override { return AccessibilityRole::Column; }
> -    
> +
>      void setColumnIndex(int columnIndex) { m_columnIndex = columnIndex; }
> -    int columnIndex() const { return m_columnIndex; }    
> -    
> +    int columnIndex() const { return m_columnIndex; }
> +
>      void addChildren() override;
>      void setParent(AccessibilityObject*) override;
> -    
> +
>      LayoutRect elementRect() const override;
> -    
> +
>  private:
>      AccessibilityTableColumn();
> -    
> +
>      AccessibilityObject* headerObjectForSection(RenderTableSection*, bool thTagRequired);
>      bool computeAccessibilityIsIgnored() const override;

Please revert whitespace changes.

[Eike Rathke]

(In reply to Michael Catanzaro from comment #7)
> 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 can only see bug #186798 and get "You are not authorized to access" for the others. However, that one bug and patch was submitted by Tomas (I wasn't aware that old bug still existed) and my patch contains the same old fixes if they weren't applied, as I tried to apply everything we have to current master, plus a set of new ones resulting from differences between 2.20.x and 2.22.7

> 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.
Do you want me to split the patch?

> 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.
That's probably due to an editor not saving trailing spaces (which is a good thing, IMHO). And also already in the old patch. But well, yes, that's easily squeezable to the one changed line.

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


> > 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.
Agreed. That was also discussed in bug #186798. So, what I could do to prevent duplicated discussion and work is to split off all the old changes and submit a new patch that contains only newer fixes. Does that make sense?


> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:154
> > +        ResultList result { };
> 
> But ResultList is:
> 
> Vector<Value*, 1>
> 
> so this one shouldn't need the brace initialization... correct?
Looks like. That one is also carried over from bug #186798, maybe it was simply "one too much" (three had to be initialized, one not).

> > 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.
Ditto, both from the old patch.

> > 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.
Bitfield initializers are available only since C++20


(In reply to Alexey Proskuryakov from comment #11)
> > 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.
> 
> It may be obvious to everyone looking at this bug, but adding initialization
> where it is not needed regresses performance, almost by definition. Other
> changes needed to please code analysis tools can be introducing other
> problems, so I think that each one needs to be discussed on its merits.
If so, then WebKit should be added to the actual Coverity Scan Analysis platform, where each "error" can be marked as false positive (which is remembered) and other means of telling Coverity already in the source code how to interpret things exist. For example, there are dozens of false positives about unreachable code or uninitialized where, well, an ASSERT_NOTREACHED() or so exists in a switch case. Coverity can be taught about such things.

[Filip Pizlo]

(In reply to Eike Rathke from comment #13)
> (In reply to Michael Catanzaro from comment #7)
> > 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 can only see bug #186798 and get "You are not authorized to access" for
> the others. However, that one bug and patch was submitted by Tomas (I wasn't
> aware that old bug still existed) and my patch contains the same old fixes
> if they weren't applied, as I tried to apply everything we have to current
> master, plus a set of new ones resulting from differences between 2.20.x and
> 2.22.7
> 
> > 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.
> Do you want me to split the patch?
> 
> > 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.
> That's probably due to an editor not saving trailing spaces (which is a good
> thing, IMHO). And also already in the old patch. But well, yes, that's
> easily squeezable to the one changed line.
> 
> > BTW, welcome Eike. :) Darin, Eike is the new maintainer of Red Hat's WebKit
> > and Chromium packages.
> Thanks :-)
> 
> 
> > > 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.
> Agreed. That was also discussed in bug #186798. So, what I could do to
> prevent duplicated discussion and work is to split off all the old changes
> and submit a new patch that contains only newer fixes. Does that make sense?
> 
> 
> > > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:154
> > > +        ResultList result { };
> > 
> > But ResultList is:
> > 
> > Vector<Value*, 1>
> > 
> > so this one shouldn't need the brace initialization... correct?
> Looks like. That one is also carried over from bug #186798, maybe it was
> simply "one too much" (three had to be initialized, one not).
> 
> > > 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.
> Ditto, both from the old patch.
> 
> > > 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.
> Bitfield initializers are available only since C++20
> 
> 
> (In reply to Alexey Proskuryakov from comment #11)
> > > 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.
> > 
> > It may be obvious to everyone looking at this bug, but adding initialization
> > where it is not needed regresses performance, almost by definition. Other
> > changes needed to please code analysis tools can be introducing other
> > problems, so I think that each one needs to be discussed on its merits.
> If so, then WebKit should be added to the actual Coverity Scan Analysis
> platform, where each "error" can be marked as false positive (which is
> remembered) and other means of telling Coverity already in the source code
> how to interpret things exist. For example, there are dozens of false
> positives about unreachable code or uninitialized where, well, an
> ASSERT_NOTREACHED() or so exists in a switch case. Coverity can be taught
> about such things.

Compiler dude here.  Adding initialization where it is unnecessary is most likely not going to add any overhead.  It won't even add instructions in many cases.

This is because once you get to the compiler's optimizer, it knows very well if an initialization is live at any use. If an initialization was added just to placate a sanitizer or static analysis tool, then it will be very obvious to the compiler that the initialization is dead. The compiler will then remove it.

For example, say you have:

int x = 0;
x = 42;
print(x);

Then the "x = 0" part will disappear in the eventual code. This is true even for complex control flow patterns, including probably many cases where we add " = 0" to spoonfeed tools around a switch statement.

Generally, I only like to spoonfeed tools if those tools are useful.  I think that in the case of adding initializations of fields, it's not just useful to do it but it's also generally perf-neutral and it makes the code more clear.

[Eike Rathke]

(In reply to Eike Rathke from comment #13)
> > > 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.
> Ditto, both from the old patch.

Or rather, it's old here, but not in bug #186798 (which carries only
changes to Source/JavaScriptCore/), maybe in a patch at one of the other
bugs I can't see?

[Alexey Proskuryakov]

> If so, then WebKit should be added to the actual Coverity Scan Analysis platform, where each "error" can be marked as false positive

Are you talking about <https://scan.coverity.com/projects/webkit>, or some other Coverity platform thing?

> Compiler dude here.  Adding initialization where it is unnecessary is most likely not going to add any overhead.  It won't even add instructions in many cases.

Most of the code that Coverity complains about is not dead code of the kind you describe. Let's look at the changes to GradientImage for example, are you claiming that the compiler is going to eliminate the unnecessary initialization added in this patch?

[Eike Rathke]

(In reply to Alexey Proskuryakov from comment #16)
> > If so, then WebKit should be added to the actual Coverity Scan Analysis platform, where each "error" can be marked as false positive
> 
> Are you talking about <https://scan.coverity.com/projects/webkit>, or some
> other Coverity platform thing?
No, exactly that. But then again.. is it really scanned?

Component Name  Pattern         Ignore  Line of Code    Defect density
Source          ./Source        No      0               N/A

My guess: ./Source should be ./Source/* instead (or even /Source/.*
maybe?), or there's something else preventing the scan.

[Michael Catanzaro]

(In reply to Eike Rathke from comment #13)
> Do you want me to split the patch?

Yes please. Such a small patch will be very easy to land, anyway.

> Agreed. That was also discussed in bug #186798. So, what I could do to
> prevent duplicated discussion and work is to split off all the old changes
> and submit a new patch that contains only newer fixes. Does that make sense?

Yeah that sounds like a good idea. You can take over and update Tom's patch there.

[Michael Catanzaro]

(In reply to Alexey Proskuryakov from comment #16)
> Most of the code that Coverity complains about is not dead code of the kind
> you describe. Let's look at the changes to GradientImage for example, are
> you claiming that the compiler is going to eliminate the unnecessary
> initialization added in this patch?

It's not unnecessary, though. It's fixing a real bug. The first time GradientImage::drawPattern is called, m_cachedGeneratorHash is read uninitialized. The only place m_cachedGeneratorHash is ever initialized is after that read, so currently there's no way it can ever be initialized unless an uninitialized memory read has already occurred.

I don't like Coverity, but got to admit the scan results are quality....

[Michael Catanzaro]

(In reply to Eike Rathke from comment #13)
> If so, then WebKit should be added to the actual Coverity Scan Analysis
> platform, where each "error" can be marked as false positive (which is
> remembered) and other means of telling Coverity already in the source code
> how to interpret things exist. For example, there are dozens of false
> positives about unreachable code or uninitialized where, well, an
> ASSERT_NOTREACHED() or so exists in a switch case. Coverity can be taught
> about such things.

Maybe something like this would be enough to solve it:

diff --git a/Source/WTF/wtf/Assertions.h b/Source/WTF/wtf/Assertions.h
index ac1a0774595..f5477d6008c 100644
--- a/Source/WTF/wtf/Assertions.h
+++ b/Source/WTF/wtf/Assertions.h
@@ -64,7 +64,7 @@ extern "C" void _ReadWriteBarrier(void);
 #endif
 #endif
 
-#ifdef NDEBUG
+#if defined(NDEBUG) && !defined(__COVERITY__)
 /* Disable ASSERT* macros in release mode. */
 #define ASSERTIONS_DISABLED_DEFAULT 1
 #els

[Alexey Proskuryakov]

> No, exactly that. But then again.. is it really scanned?

I do not know. The access is restricted, and IIRC I never had access. Worth discussing on the webkit-security list, not in this bug.

> It's not unnecessary, though. It's fixing a real bug. The first time
> GradientImage::drawPattern is called, m_cachedGeneratorHash is read
> uninitialized.

The code is actually correct. The first time this function is called, m_cachedImage is null, so the rest of the expression is not evaluated (it's called short-circuiting logical operators in C++).

[Michael Catanzaro]

(In reply to Alexey Proskuryakov from comment #21)
> The code is actually correct. The first time this function is called,
> m_cachedImage is null, so the rest of the expression is not evaluated (it's
> called short-circuiting logical operators in C++).

Ah, you are right, of course. It is subtle and very fragile, though. I wouldn't want to leave a footgun like that in WebKit. Programming is just too hard if we can't count on the data members of a class to be initialized.

[Darin Adler]

(In reply to Michael Catanzaro from comment #22)
> (In reply to Alexey Proskuryakov from comment #21)
> > The code is actually correct. The first time this function is called,
> > m_cachedImage is null, so the rest of the expression is not evaluated (it's
> > called short-circuiting logical operators in C++).
> 
> Ah, you are right, of course. It is subtle and very fragile, though. I
> wouldn't want to leave a footgun like that in WebKit. Programming is just
> too hard if we can't count on the data members of a class to be initialized.

I understand your point of view on this Michael, and this is the coding style discussion I was alluding to.

[Darin Adler]

Comment on attachment 367314 [details]
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.

[Alexey Proskuryakov]

I think that some ways to help inform the coding style discussion are:

1. Confirm whether it is practical to silence incorrect findings on the website. 

2. Think about whether there are less costly way to find such bugs. E.g. one thing that we haven’t done to my knowledge is to run regression tests with UBSan. Does that find most of the same uninitialized variable bugs? Does it find more? Static analysis is not the same thing in the abstract, but what about practice?

I am skeptical of the line of thinking that it’s ok to slow down code as long as we can’t measure that with benchmarks. Benchmarks are not measuring everything, and their results are quite coarse. WebKit developers have always been micro-optimizing code they wrote, and I think that’s been doing us good overall.

[Michael Catanzaro]

We're now talking about extreme micro-optimization at the cost of safety and robustness. The case we discussed up above, in comment #21, seems like case in point to me. Although you're right that the code is correct in its current form -- I was wrong about that -- it's easy to see a small and seemingly-correct change to this code would result in uninitialized memory read. Short-circuiting to avoid e.g. dereferencing NULL is common practice; short-circuiting to avoid uninitialized memory reads is not. The checks could easily be moved around in the future. Or a getter function could be added to the class. It's just too fragile. Humans are not good enough to avoid such mistakes; we can check to make sure local variables we declare in functions are initialized before first use, but it's just too hard if we can't assume that classes are fully-initialized.

[Alexey Proskuryakov]

It’s not just this case. Darin went through the whole patch and pointed out lots of those. 

Michael, are you saying that you’ve made up your mind, and your opinion cannot be changed regardless to what the answers to my two questions are?

[Michael Catanzaro]

I would just initialize everything regardless, yes.

[Eike Rathke]

Oh, you sort not initializing under coding style, ah well.. wasn't aware of that.


(In reply to Darin Adler from comment #24)
> > 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.
As I mentioned, part of the patch are old covscan fixes that more or less still
applied, if current code initializes things now then that is not caught and the
patch is moot. Our Covscan didn't run against master or versions more recent
than 2.22

This (patch outdated) is in particluar true for most if not all
Source/JavaScriptCore/ changes that were already submitted with bug #186798 or
changes submitted with bug #190468 (both which I now have access to, thanks).

As mentioned, I'll submit a new patch that omits those old changes and contains
only newer fixes to not duplicate work, and I'll check the changes against
these comments before submitting and weed out the "style only" ones.


> > 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.
Current code on master has
AudioProcessingEvent::AudioProcessingEvent() = default;
so either initializing with 0.0 is correct (and could be omitted) or it needs
an explicit different value anyway.


> > Source/WebCore/animation/DeclarativeAnimation.h:95
> > +    double m_previousIteration { 0.0 };
> 
> I think NAN is a better default here than 0. Not sure.
It's used in DeclarativeAnimation::invalidateDOMEvents() line 284

  else if (wasActive && isActive && m_previousIteration != iteration)

where a NaN would compare to always false with operator!=(), which in this case
would be ok. (if no one introduces comparisons with operator<() or operator>())

[Darin Adler]

(In reply to Eike Rathke from comment #29)
> Oh, you sort not initializing under coding style, ah well.. wasn't aware of
> that.

That’s an incorrect characterization of my view. Here’s my view:

Initializing something that is *used* is absolutely required, not a coding style decision. Many of the changes in this patch repair those kinds of problems and we must take those changes as soon as possible.

Initializing something that is *not* used is a coding style decision. Some advocate this style, citing that it’s safer to initialize everything and is unlikely to have any major negative impact or cost. Others advocate a style where we initialize only in cases where the initialization is meaningful, citing possible performance benefits in cases where the compiler can’t see the fact that the value is never used, or preferring not to write code that has no effect by setting an initial value that is provably irrelevant since it is only written and never read.

[Eike Rathke]

So, taking only my newer changes and weeding out everything marked "not needed" above leaves just the NaN-or-not-NaN case

--- webkitgtk-2.22.7/Source/WebCore/animation/DeclarativeAnimation.h.covscan_uninit_ctor	2019-02-28 11:08:20.000000000 +0100
+++ webkitgtk-2.22.7/Source/WebCore/animation/DeclarativeAnimation.h	2019-04-04 00:36:57.991556884 +0200
@@ -69,7 +69,7 @@ private:
     Ref<Animation> m_backingAnimation;
     bool m_wasPending { false };
     AnimationEffectReadOnly::Phase m_previousPhase { AnimationEffectReadOnly::Phase::Idle };
-    double m_previousIteration;
+    double m_previousIteration { 0.0 };
     GenericEventQueue m_eventQueue;
 };
 

I guess this bug can be closed then. And sorry for fuzz.

[Darin Adler]

To be clear, I am not intending to forbid check-in of the ones I marked "not needed". Instead my intention is to call them out as ones where we have to think/discuss at least a little more to make our "project style" decision. Also, my analysis may be wrong.

[Michael Catanzaro]

Well I think we've discussed it. Alexey seems to be opposed (or at least unenthusiastic) and I won't give r+ unless he's OK with it.

Let's land the DeclarativeAnimation fix, at least.

[Darin Adler]

(In reply to Michael Catanzaro from comment #33)
> Let's land the DeclarativeAnimation fix, at least.

What about the HTMLMenuElement one, for example? I want that one!

[Michael Catanzaro]

Eike, might be a good idea to go through these again and see if you missed anything else.

[Eike Rathke]

Sigh.. only bug #186798 had a patch attached, the others only covscan logs, so concluding only from my last changes didn't include the ones related. I'll prepare another patch that does not cross #186798 but includes everything else, modulo the trailing blank change.

[Eike Rathke]

(In reply to Darin Adler from comment #4)
> > Source/WebCore/html/HTMLMediaElement.cpp:2105
> > -        if (actionIfInvalid == Complain)
> > +        if (actionIfInvalid == Complain) {
> >              FrameLoader::reportLocalLoadFailed(frame.get(), url.stringCenterEllipsizedToLength());
> >              ERROR_LOG(LOGIDENTIFIER, url , " was rejected by SecurityOrigin");
> > +        }
> >          return false;
> 
> Good fix, but a different Coverity error, not uninitialized data members.

I submitted that with a separate bug #196933.

[Alexey Proskuryakov]

To be clear, I am quite interested in better undefined behavior diagnostics, as well as in other bugs that static analysis catches. I’m questioning whether the approach that has a negative user impact in terms of performance is the one to choose.

The correct place for such discussions is the mailing list of course, as a discussion in a bug is only immediately visible to people CCed on it.

[Eike Rathke]

Comment on attachment 367314 [details]
Patch

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

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

AccessibilityTableColumn::headerObjectForSection() does

    unsigned numCols = section->numColumns();
    if (m_columnIndex >= numCols)
        return nullptr;

I guess any other initial value than 0 (if nothing was set with setColumnIndex()) would be unexpected.
Also

    for (int testCol = m_columnIndex; testCol >= 0; --testCol) {

starting with a greater value then would seem quite odd to me.

>> Source/WebCore/animation/DeclarativeAnimation.h:95
>> +    double m_previousIteration { 0.0 };
> 
> I think NAN is a better default here than 0. Not sure.

DeclarativeAnimation::invalidateDOMEvents() has

        else if (wasActive && isActive && m_previousIteration != iteration) {
            auto iterationBoundary = iteration;
            if (m_previousIteration > iteration)
                iterationBoundary++;

With NaN m_previousIteration != iteration is always true, m_previousIteration > iteration is always false, no matter what the value of iteration is (even if negative).
Just to point out, don't know what would be correct.

[Eike Rathke]

Created attachment 367526 [details]
Patch

[Eike Rathke]

This patch omits all changes already present on bug #186798.