Bug 56196 – Some class members were used prior to initialization

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Mumford]

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

[Alexey Proskuryakov]

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.

[Chris Mumford]

(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.

[Alexey Proskuryakov]

Comment on attachment 85478 [details]
Patch with member initialization to default values.

It should be r?, not r+.

[Chris Mumford]

(In reply to comment #3)
> (From update of attachment 85478 [details])
> It should be r?, not r+.

Oops! thx again.

[Adam Barth]

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)

!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 85478 [details]:

transitions/interrupted-accelerated-transition.html bug 56242 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> > 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.