Bug 56196

Summary: Some class members were used prior to initialization
Product: WebKit Reporter: Chris Mumford <chris.mumford>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: ap, commit-queue, mdelaney7
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch with member initialization to default values. none

Chris Mumford
Reported 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
Attachments
Patch with member initialization to default values. (3.53 KB, patch)
2011-03-11 08:29 PST, Chris Mumford 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 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.
Chris Mumford
Comment 2 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.
Alexey Proskuryakov
Comment 3 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+.
Chris Mumford
Comment 4 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.
Adam Barth
Comment 5 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) !
WebKit Commit Bot
Comment 6 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.
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2011-03-15 03:47:50 PDT
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.