[Coverity] Fix uninitialized constructor defects in .../html
Created attachment 120097 [details] Patch
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?
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.
Created attachment 120110 [details] Patch
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.
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.
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.
Created attachment 120190 [details] Patch
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?
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).
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.
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.
CC'ing folks who show up in svn blame for WebGLGetInfo.cpp
Comment on attachment 120190 [details] Patch The changes to WebGLGetInfo look good.
Good to go?
Created attachment 120985 [details] Patch
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.
> 103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it? > at /usr/lib/git-core/git-svn line 5210 Sorry. Will fix.
Comment on attachment 120985 [details] Patch Attachment 120985 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11082160
Comment on attachment 120985 [details] Patch Attachment 120985 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11083171
Comment on attachment 120985 [details] Patch Attachment 120985 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11070201
Comment on attachment 120985 [details] Patch Attachment 120985 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10954873
Were the bots misfiring on this patch? Or shall I resync and try again?
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.
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.
Created attachment 121164 [details] Patch
Now fixed.
Can you send this into the commit queue? Or do I need to merge to head at this point? Thanks!
Comment on attachment 121164 [details] Patch We'll see if it still applies.
Comment on attachment 121164 [details] Patch Clearing flags on attachment: 121164 Committed r105457: <http://trac.webkit.org/changeset/105457>
All reviewed patches have been landed. Closing bug.