Bug 24684 – Clean up WebCore based on Coverty Prevent static analysis results

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

[Eric Seidel (no email)]

Clean up WebCore based on Coverty Prevent static analysis results

See attached patch.

[Simon Fraser (smfr)]

There is no attached patch.

[Eric Seidel (no email)]

Yup.  Patch had an issue, will be attached soon...

[Eric Seidel (no email)]

Created attachment 28756 [details]
First pass cleanup with changelog, all tests pass

 WebCore/ChangeLog                                  |  107 +++++++++++++++++
 WebCore/css/CSSFontSelector.cpp                    |   12 ++-
 WebCore/css/CSSStyleSelector.cpp                   |    6 +
 WebCore/css/CSSStyleSheet.cpp                      |    7 +-
 WebCore/css/SVGCSSStyleSelector.cpp                |  121 +++++---------------
 WebCore/dom/ContainerNode.cpp                      |   14 +-
 WebCore/dom/Document.cpp                           |    2 -
 WebCore/editing/ApplyStyleCommand.cpp              |   10 +-
 .../editing/InsertParagraphSeparatorCommand.cpp    |    3 +-
 WebCore/editing/markup.cpp                         |    3 +-
 WebCore/html/CanvasStyle.cpp                       |   13 ++-
 WebCore/loader/TextDocument.cpp                    |   13 ++-
 WebCore/page/FocusController.cpp                   |    6 +-
 WebCore/page/animation/AnimationBase.h             |    5 +-
 WebCore/page/animation/AnimationController.cpp     |    1 +
 WebCore/page/animation/ImplicitAnimation.cpp       |    3 +-
 WebCore/page/animation/ImplicitAnimation.h         |    4 +-
 WebCore/page/animation/KeyframeAnimation.cpp       |    2 +-
 WebCore/page/animation/KeyframeAnimation.h         |    4 +-
 WebCore/platform/chromium/PasteboardChromium.cpp   |   19 ++--
 WebCore/platform/graphics/GlyphPageTreeNode.cpp    |    1 +
 .../transforms/PerspectiveTransformOperation.cpp   |    5 +-
 .../transforms/RotateTransformOperation.cpp        |    8 +-
 WebCore/rendering/RenderLayer.cpp                  |   12 +-
 WebCore/rendering/RenderTableSection.cpp           |    8 +-
 WebCore/rendering/SVGRenderSupport.cpp             |    2 +-
 WebCore/rendering/bidi.cpp                         |    3 +-
 27 files changed, 226 insertions(+), 168 deletions(-)

[Mark Rowe (bdash)]

Please split this up. Many of these changes are completely unrelated.

[Sam Weinig]

Comment on attachment 28756 [details]
First pass cleanup with changelog, all tests pass

r- based on Marks comment.

[Eric Seidel (no email)]

Comment on attachment 28756 [details]
First pass cleanup with changelog, all tests pass

I've split this into a bunch of patches.  Attaching...

[Eric Seidel (no email)]

Created attachment 28899 [details]
Coverty fixes in Animations/Transforms

 WebCore/ChangeLog                                  |   29 ++++++++++++++++++++
 WebCore/page/animation/AnimationBase.h             |    5 +--
 WebCore/page/animation/AnimationController.cpp     |    1 +
 WebCore/page/animation/ImplicitAnimation.cpp       |    3 +-
 WebCore/page/animation/ImplicitAnimation.h         |    4 +-
 WebCore/page/animation/KeyframeAnimation.cpp       |    2 +-
 WebCore/page/animation/KeyframeAnimation.h         |    4 +-
 .../transforms/PerspectiveTransformOperation.cpp   |    5 +---
 .../transforms/RotateTransformOperation.cpp        |    8 +----
 9 files changed, 41 insertions(+), 20 deletions(-)

[Eric Seidel (no email)]

Created attachment 28900 [details]
Font fallback cleanup and added ASSERT for GlyphPageTreeNode

 WebCore/ChangeLog                               |   17 +++++++++++++++++
 WebCore/css/CSSFontSelector.cpp                 |   12 ++++++++----
 WebCore/platform/graphics/GlyphPageTreeNode.cpp |    1 +
 3 files changed, 26 insertions(+), 4 deletions(-)

[Eric Seidel (no email)]

Created attachment 28901 [details]
CSS dead code removal and cleanup from Coverty errors

 WebCore/ChangeLog                   |   24 +++++++
 WebCore/css/CSSStyleSelector.cpp    |    6 ++
 WebCore/css/CSSStyleSheet.cpp       |    7 +-
 WebCore/css/SVGCSSStyleSelector.cpp |  121 +++++++++--------------------------
 4 files changed, 63 insertions(+), 95 deletions(-)

[Eric Seidel (no email)]

Created attachment 28902 [details]
Style cleanup and dead code removal in dom, editing

 WebCore/ChangeLog                                  |   31 ++++++++++++++++++++
 WebCore/dom/ContainerNode.cpp                      |   14 ++++----
 WebCore/dom/Document.cpp                           |    2 -
 WebCore/editing/ApplyStyleCommand.cpp              |   10 +++---
 .../editing/InsertParagraphSeparatorCommand.cpp    |    3 +-
 WebCore/editing/markup.cpp                         |    3 +-
 6 files changed, 45 insertions(+), 18 deletions(-)

[Eric Seidel (no email)]

Created attachment 28903 [details]
Fix case where lBreak.obj->isBR() when lBreak.obj was NULL

 WebCore/ChangeLog          |   15 +++++++++++++++
 WebCore/rendering/bidi.cpp |    3 +--
 2 files changed, 16 insertions(+), 2 deletions(-)

[Eric Seidel (no email)]

Created attachment 28904 [details]
Make TextTokenizer ASSERT that the buffer was freed

 WebCore/ChangeLog               |   13 +++++++++++++
 WebCore/loader/TextDocument.cpp |   13 +++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

[Eric Seidel (no email)]

Created attachment 28905 [details]
Move ASSERT(foo) to before where foo-> is used

 WebCore/ChangeLog                                |   15 +++++++++++++++
 WebCore/platform/chromium/PasteboardChromium.cpp |   19 +++++++++----------
 WebCore/rendering/SVGRenderSupport.cpp           |    2 +-
 3 files changed, 25 insertions(+), 11 deletions(-)

[Eric Seidel (no email)]

Created attachment 28906 [details]
Remove dead code and style cleanup

 WebCore/ChangeLog                        |   23 +++++++++++++++++++++++
 WebCore/html/CanvasStyle.cpp             |   13 +++++++++++--
 WebCore/page/FocusController.cpp         |    6 +++---
 WebCore/rendering/RenderLayer.cpp        |   12 ++++++------
 WebCore/rendering/RenderTableSection.cpp |    8 +++-----
 5 files changed, 46 insertions(+), 16 deletions(-)

[Simon Fraser (smfr)]

Comment on attachment 28899 [details]
Coverty fixes in Animations/Transforms

r=me

[Darin Adler]

Comment on attachment 28906 [details]
Remove dead code and style cleanup

r=me

[Darin Adler]

Comment on attachment 28901 [details]
CSS dead code removal and cleanup from Coverty errors

> +        * css/CSSStyleSheet.cpp:
> +        (WebCore::CSSStyleSheet::styleSheetChanged):
> +          Removing dead code, "root" can never be null here.

Can static_cast<CSSStyleSheet*>(root)->doc() ever be null?

[Darin Adler]

Comment on attachment 28903 [details]
Fix case where lBreak.obj->isBR() when lBreak.obj was NULL

> +        Fix case where lBreak.obj->isBR() when lBreak.obj was NULL
> +	https://bugs.webkit.org/show_bug.cgi?id=24684
> +
> +	I don't have a test case where we were taking the wrong path here
> +	However, adding an ASSERT(lBreak.obj) before this usage caused
> +	multiple bidi tests to fail, so lBreak.obj can be null here.

Tabs in ChangeLog.

> -    if (lBreak == resolver.position() && !lBreak.obj->isBR()) {
> +    if (lBreak == resolver.position() && (!lBreak.obj || !lBreak.obj->isBR())) {

Seems harmless to add this, even though it may be impossible in practice.

r=me

[Darin Adler]

Comment on attachment 28905 [details]
Move ASSERT(foo) to before where foo-> is used

I understand the desire to move asserts before the thing asserted are used, but I don't like the idea of moving all the declarations to the top of the function. Could you make a more limited version that doesn't move the definitions of renderer, cachedImage, and image up to the top of the function?

In either case, r=me

[Eric Seidel (no email)]

(In reply to comment #17)
> (From update of attachment 28901 [details] [review])
> > +        * css/CSSStyleSheet.cpp:
> > +        (WebCore::CSSStyleSheet::styleSheetChanged):
> > +          Removing dead code, "root" can never be null here.
> 
> Can static_cast<CSSStyleSheet*>(root)->doc() ever be null?

I'm not sure.  It appears it can be, not necessarily in this case.  But I'll update the patch anyway.

[Eric Seidel (no email)]

Created attachment 28933 [details]
CSS dead code removal and cleanup from Coverty errors

 WebCore/ChangeLog                   |   24 +++++++
 WebCore/css/CSSStyleSelector.cpp    |    6 ++
 WebCore/css/CSSStyleSheet.cpp       |    2 +-
 WebCore/css/SVGCSSStyleSelector.cpp |  121 +++++++++--------------------------
 4 files changed, 61 insertions(+), 92 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 28901 [details]
CSS dead code removal and cleanup from Coverty errors

Now null-checking doc() again, per darin's suggestion.

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/41983
http://trac.webkit.org/changeset/41984
http://trac.webkit.org/changeset/41985
http://trac.webkit.org/changeset/41986
http://trac.webkit.org/changeset/41988
http://trac.webkit.org/changeset/41989
http://trac.webkit.org/changeset/41990
http://trac.webkit.org/changeset/41991