Bug 56196 - Some class members were used prior to initialization
Summary: Some class members were used prior to initialization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-11 08:29 PST by Chris Mumford
Modified: 2011-03-15 08:42 PDT (History)
3 users (show)

See Also:


Attachments
Patch with member initialization to default values. (3.53 KB, patch)
2011-03-11 08:29 PST, Chris Mumford
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Mumford 2011-03-11 08:29:37 PST
Created attachment 85478 [details]
Patch with member initialization to default values.

When running WebKit in Valgrind several class members were reported as being read prior to being initialized. Specifically:

WebCore::AccessibilityImageMapLink::m_parent
WebCore::DeleteSelectionCommand::m_needPlaceholder
WebCore::HTMLCanvasElement::m_rendererIsCanvas
WebCore::XPathResult::m_nodeSetPosition
WebCore::XPathResult::m_domTreeVersion
Comment 1 Alexey Proskuryakov 2011-03-11 11:47:50 PST
This sounds pretty horrible. If Valgrind found uninitialized reads, it's possible that a regression test can be made, and in any case, more detailed information is needed. Could you please post stack traces, and steps to reproduce?

To get a patch reviewed, you need to mark it for review (r?). Please see <http://www.webkit.org/coding/contributing.html> for information about contributing code to WebKit.
Comment 2 Chris Mumford 2011-03-12 09:14:42 PST
(In reply to comment #1)
> This sounds pretty horrible. If Valgrind found uninitialized reads, it's possible that a regression test can be made, and in any case, more detailed information is needed. Could you please post stack traces, and steps to reproduce?
> 
> To get a patch reviewed, you need to mark it for review (r?). Please see <http://www.webkit.org/coding/contributing.html> for information about contributing code to WebKit.

Thanks Alexey for the comment and advise. I've had these initializers in our code for a while now (>12 mo.) and am just now getting around to submitting a patch. In an effort to reproduce one (or more) Valgrind errors I did uncomment the initializations and ran with Valgrind, but did not get any warnings. I don't know if these issues depend on the web page being loaded, but I suspect they would.

I think that it's always a good idea to have all member initialized to a safe reasonable default value (certainly not random memory). I'll mark this for review and if it is rejected for lack of clear evidence or regression tests then I'll take it from there.
Comment 3 Alexey Proskuryakov 2011-03-12 09:59:55 PST
Comment on attachment 85478 [details]
Patch with member initialization to default values.

It should be r?, not r+.
Comment 4 Chris Mumford 2011-03-12 12:17:02 PST
(In reply to comment #3)
> (From update of attachment 85478 [details])
> It should be r?, not r+.

Oops! thx again.
Comment 5 Adam Barth 2011-03-15 02:28:49 PDT
Comment on attachment 85478 [details]
Patch with member initialization to default values.

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

I'm hesitating to accept this patch without tests.  Usually we require all patches to have associated tests, if they change any observable behavior.  In this case, you should be able to write a test that trigger the UMRs.  However, I'm marking this patch r+ because I'd like to encourage you to upstream more patches.  Thanks for the patch.

> Source/WebCore/accessibility/AccessibilityImageMapLink.cpp:45
> +    , m_parent(0)

wow!

> Source/WebCore/xml/XPathResult.cpp:45
> +    , m_nodeSetPosition(0)
> +    , m_domTreeVersion(0)

!
Comment 6 WebKit Commit Bot 2011-03-15 03:45:35 PDT
The commit-queue encountered the following flaky tests while processing attachment 85478 [details]:

transitions/interrupted-accelerated-transition.html bug 56242 (authors: simon.fraser@apple.com and tonyg@chromium.org)
The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2011-03-15 03:47:45 PDT
Comment on attachment 85478 [details]
Patch with member initialization to default values.

Clearing flags on attachment: 85478

Committed r81128: <http://trac.webkit.org/changeset/81128>
Comment 8 WebKit Commit Bot 2011-03-15 03:47:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Alexey Proskuryakov 2011-03-15 08:42:56 PDT
> > Source/WebCore/xml/XPathResult.cpp:45
> > +    , m_nodeSetPosition(0)
> > +    , m_domTreeVersion(0)
> 
> !

Valgrind was probably just confused about these. I'm less sure about others.