Bug 24684 - Clean up WebCore based on Coverty Prevent static analysis results
Summary: Clean up WebCore based on Coverty Prevent static analysis results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-18 16:39 PDT by Eric Seidel (no email)
Modified: 2009-03-25 15:16 PDT (History)
4 users (show)

See Also:


Attachments
First pass cleanup with changelog, all tests pass (39.24 KB, patch)
2009-03-19 11:42 PDT, Eric Seidel (no email)
sam: review-
Details | Formatted Diff | Diff
Coverty fixes in Animations/Transforms (8.18 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
Font fallback cleanup and added ASSERT for GlyphPageTreeNode (4.07 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
CSS dead code removal and cleanup from Coverty errors (10.47 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Style cleanup and dead code removal in dom, editing (7.71 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
Fix case where lBreak.obj->isBR() when lBreak.obj was NULL (1.46 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff
Make TextTokenizer ASSERT that the buffer was freed (1.84 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff
Move ASSERT(foo) to before where foo-> is used (3.20 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff
Remove dead code and style cleanup (6.20 KB, patch)
2009-03-24 13:12 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff
CSS dead code removal and cleanup from Coverty errors (10.28 KB, patch)
2009-03-25 10:19 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-03-18 16:39:02 PDT
Clean up WebCore based on Coverty Prevent static analysis results

See attached patch.
Comment 1 Simon Fraser (smfr) 2009-03-18 18:19:02 PDT
There is no attached patch.
Comment 2 Eric Seidel (no email) 2009-03-18 18:40:05 PDT
Yup.  Patch had an issue, will be attached soon...
Comment 3 Eric Seidel (no email) 2009-03-19 11:42:33 PDT
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(-)
Comment 4 Mark Rowe (bdash) 2009-03-19 16:06:59 PDT
Please split this up. Many of these changes are completely unrelated.
Comment 5 Sam Weinig 2009-03-23 11:09:48 PDT
Comment on attachment 28756 [details]
First pass cleanup with changelog, all tests pass

r- based on Marks comment.
Comment 6 Eric Seidel (no email) 2009-03-24 13:12:02 PDT
Comment on attachment 28756 [details]
First pass cleanup with changelog, all tests pass

I've split this into a bunch of patches.  Attaching...
Comment 7 Eric Seidel (no email) 2009-03-24 13:12:24 PDT
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(-)
Comment 8 Eric Seidel (no email) 2009-03-24 13:12:27 PDT
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(-)
Comment 9 Eric Seidel (no email) 2009-03-24 13:12:29 PDT
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(-)
Comment 10 Eric Seidel (no email) 2009-03-24 13:12:32 PDT
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(-)
Comment 11 Eric Seidel (no email) 2009-03-24 13:12:34 PDT
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(-)
Comment 12 Eric Seidel (no email) 2009-03-24 13:12:37 PDT
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(-)
Comment 13 Eric Seidel (no email) 2009-03-24 13:12:39 PDT
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(-)
Comment 14 Eric Seidel (no email) 2009-03-24 13:12:42 PDT
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(-)
Comment 15 Simon Fraser (smfr) 2009-03-24 13:19:55 PDT
Comment on attachment 28899 [details]
Coverty fixes in Animations/Transforms

r=me
Comment 16 Darin Adler 2009-03-24 14:14:07 PDT
Comment on attachment 28906 [details]
Remove dead code and style cleanup

r=me
Comment 17 Darin Adler 2009-03-24 14:19:10 PDT
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?
Comment 18 Darin Adler 2009-03-24 14:20:00 PDT
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
Comment 19 Darin Adler 2009-03-24 14:22:28 PDT
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
Comment 20 Eric Seidel (no email) 2009-03-24 14:58:14 PDT
(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.
Comment 21 Eric Seidel (no email) 2009-03-25 10:19:18 PDT
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(-)
Comment 22 Eric Seidel (no email) 2009-03-25 10:20:04 PDT
Comment on attachment 28901 [details]
CSS dead code removal and cleanup from Coverty errors

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