Bug 74965 – [Coverity] Fix uninitialized constructor defects in .../html

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

[Coverity] Fix uninitialized constructor defects in .../html

[Greg Billock]

Created attachment 120097 [details]
Patch

[Adam Barth]

Comment on attachment 120097 [details]
Patch

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

> Source/WebCore/html/HTMLFormCollection.cpp:48
>      : HTMLCollection(form.get(), OtherCollection, formCollectionInfo(form.get()))
> +    , currentPos(0)

Can you reformat this initializer list to the usual WebKit style (which is to start each line after the first with a ",") ?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:222
> +    , m_lineDashOffset(other.m_lineDashOffset)

Is this change testable?

[Greg Billock]

Comment on attachment 120097 [details]
Patch

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

>> Source/WebCore/html/HTMLFormCollection.cpp:48
>> +    , currentPos(0)
> 
> Can you reformat this initializer list to the usual WebKit style (which is to start each line after the first with a ",") ?

Do you mean put the superclass constructor call on the previous line?

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:222
>> +    , m_lineDashOffset(other.m_lineDashOffset)
> 
> Is this change testable?

I think so. This code gets exercised in save(), which is called by clearRect(). I added a clearRect call to the layout test to ensure that the value is preserved.

[Greg Billock]

Created attachment 120110 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 120110 [details]
Patch

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

It's great that Coverity found a real bug. We shouldn't take any complaint it makes as a requirement though. The changes in WebGLGetInfo look clearly against the intended design of this class.

> Source/WebCore/html/StepRange.cpp:42
> -    if (element->hasAttribute(precisionAttr)) {
> -        step = 1.0;
> +    step = 1.0;
> +    if (element->hasAttribute(precisionAttr))
>          hasStep = !equalIgnoringCase(element->getAttribute(precisionAttr), "float");
> -    } else
> +    else
>          hasStep = element->getAllowedValueStep(&step);

Is this change testable?

Although you're just moving this code, please note that WebKit coding style asks to not use .0 suffix on floating point literals. The same issue is present in some code you added below.

> Source/WebCore/html/canvas/CanvasStyle.h:91
> +            CMYKAValues() : c(0.0), m(0.0), y(0.0), k(0.0), a(0.0) { }

No .0, again.

> Source/WebCore/html/track/WebVTTParser.cpp:113
> +    , m_currentStartTime(0.0)
> +    , m_currentEndTime(0.0)

No .0.

[Adam Barth]

Comment on attachment 120097 [details]
Patch

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

>>> Source/WebCore/html/HTMLFormCollection.cpp:48
>>> +    , currentPos(0)
>> 
>> Can you reformat this initializer list to the usual WebKit style (which is to start each line after the first with a ",") ?
> 
> Do you mean put the superclass constructor call on the previous line?

Oh, I misread the code that was already there.  Please ignore my comment.

[Greg Billock]

Comment on attachment 120110 [details]
Patch

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

>> Source/WebCore/html/StepRange.cpp:42
>>          hasStep = element->getAllowedValueStep(&step);
> 
> Is this change testable?
> 
> Although you're just moving this code, please note that WebKit coding style asks to not use .0 suffix on floating point literals. The same issue is present in some code you added below.

Done.

I don't think this change is visible in tests. Coverity is worried that element->getAllowedValueStep may not initialize |step|, leaving the class in an undefined state. There's a check in clampValue accounting for that, so initializing is more of a cleanliness issue and insurance against regression.

>> Source/WebCore/html/canvas/CanvasStyle.h:91
>> +            CMYKAValues() : c(0.0), m(0.0), y(0.0), k(0.0), a(0.0) { }
> 
> No .0, again.

Done. (And elsewhere)

>> Source/WebCore/html/track/WebVTTParser.cpp:113
>> +    , m_currentEndTime(0.0)
> 
> No .0.

Done.

[Greg Billock]

Created attachment 120190 [details]
Patch

[Darin Adler]

Comment on attachment 120190 [details]
Patch

Many of these changes look like they make the tool happy by initializing data that will never be read. Is there a distinction between changes that are actually needed and ones that just quiet the tool?

[Alexey Proskuryakov]

Comment on attachment 120190 [details]
Patch

Thank you! The change in StepRange constructor seems somewhat arbitrary and fragile, but the benefit of making future runs of Coverity cleaner outweighs the cost of code churn in my opinion.

I'm still concerned by the changes to WebGLGetInfo. I think that these need to be discussed with an expert on this code (likely an author).

[Alexey Proskuryakov]

I think that all changes here are just to shut up the tool, except for the one that has a test. With the possible exception of WebGLGetInfo changes, these look harmless to me.

[Greg Billock]

Yes, the CanvasRenderingContext2D case is a real bug. I think the HTMLTreeBuilder and CSSPreloadScanner cases are oversights that's fair to call a hit as well. The rest are as you describe: nice to have the tool low-noise so real bugs like this stand out, and probably clearer on balance as well. I'm happy to get the WebGLInfo author to weigh in on those changes, since they are on the edge.

[Alexey Proskuryakov]

CC'ing folks who show up in svn blame for WebGLGetInfo.cpp

[Kenneth Russell]

Comment on attachment 120190 [details]
Patch

The changes to WebGLGetInfo look good.

[Greg Billock]

Good to go?

[Greg Billock]

Created attachment 120985 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 3a7674dadd24ecd6a1b023eba203ecf2ed62fc59 != edb861612940a3244431d239dacdbc36317b627c
rereading e1e2538cff7d320c2b7cc22c0b75c9369ce9cdb2
	A	LayoutTests/svg/custom/webkit-transform-crash.html
	A	LayoutTests/svg/custom/webkit-transform-crash-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/svg/SVGStyledTransformableElement.cpp
103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 158.


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

[Adam Barth]

> 103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it?
>  at /usr/lib/git-core/git-svn line 5210

Sorry.  Will fix.

[Gyuyoung Kim]

Comment on attachment 120985 [details]
Patch

Attachment 120985 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11082160

[WebKit Review Bot]

Comment on attachment 120985 [details]
Patch

Attachment 120985 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11083171

[Collabora GTK+ EWS bot]

Comment on attachment 120985 [details]
Patch

Attachment 120985 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11070201

[Early Warning System Bot]

Comment on attachment 120985 [details]
Patch

Attachment 120985 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10954873

[Greg Billock]

Were the bots misfiring on this patch? Or shall I resync and try again?

[Adam Barth]

Comment on attachment 120985 [details]
Patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:357
> +    , m_isFakeInsertionMode(false)

This variable does not exist anymore, which is what's causing the build break.

[Greg Billock]

Comment on attachment 120985 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:357
>> +    , m_isFakeInsertionMode(false)
> 
> This variable does not exist anymore, which is what's causing the build break.

drat, thanks. I saw the sync errors and thought it was a technicality.

[Greg Billock]

Created attachment 121164 [details]
Patch

[Greg Billock]

Now fixed.

[Greg Billock]

Can you send this into the commit queue? Or do I need to merge to head at this point? Thanks!

[Alexey Proskuryakov]

Comment on attachment 121164 [details]
Patch

We'll see if it still applies.

[WebKit Review Bot]

Comment on attachment 121164 [details]
Patch

Clearing flags on attachment: 121164

Committed r105457: <http://trac.webkit.org/changeset/105457>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.