56196 2011-03-11 08:29:37 -0800 Some class members were used prior to initialization 2011-03-15 08:42:56 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Major --- 0 chris.mumford webkit-unassigned ap commit-queue mdelaney7 oldest_to_newest 366015 0 85478 chris.mumford 2011-03-11 08:29:37 -0800 Created attachment 85478 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 366179 1 ap 2011-03-11 11:47:50 -0800 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. 366588 2 chris.mumford 2011-03-12 09:14:42 -0800 (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. 366593 3 85478 ap 2011-03-12 09:59:55 -0800 Comment on attachment 85478 Patch with member initialization to default values. It should be r?, not r+. 366614 4 chris.mumford 2011-03-12 12:17:02 -0800 (In reply to comment #3) > (From update of attachment 85478 [details]) > It should be r?, not r+. Oops! thx again. 367583 5 85478 abarth 2011-03-15 02:28:49 -0700 Comment on attachment 85478 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) ! 367605 6 commit-queue 2011-03-15 03:45:35 -0700 The commit-queue encountered the following flaky tests while processing attachment 85478: 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. 367608 7 85478 commit-queue 2011-03-15 03:47:45 -0700 Comment on attachment 85478 Patch with member initialization to default values. Clearing flags on attachment: 85478 Committed r81128: <http://trac.webkit.org/changeset/81128> 367609 8 commit-queue 2011-03-15 03:47:50 -0700 All reviewed patches have been landed. Closing bug. 367674 9 ap 2011-03-15 08:42:56 -0700 > > 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. 85478 2011-03-11 08:29:37 -0800 2011-03-15 03:47:45 -0700 Patch with member initialization to default values. initmembers.patch text/plain 3614 chris.mumford SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDgwODUzKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjIgQEAKKzIwMTEtMDMtMTEgIENocmlzIE11 bWZvcmQgIDxjaHJpcy5tdW1mb3JkQHBhbG0uY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIEluaXRpYWxpemluZyBzZXZlcmFsIG1lbWJlciB2YXJp YWJsZXMgdGhhdCB3ZXJlIG5vdCBpbml0aWFsaXplZCBpbgorICAgICAgICB0aGVpciBjb25zdHJ1 Y3RvcnMuIFRoZXNlIHZhbHVlcyB3ZXJlIGFsbCByZWFkIHByaW9yIHRvIGluaXRpYWxpemF0aW9u CisgICAgICAgIGFzIHJlcG9ydGVkIGJ5IFZhbGdyaW5kLgorCisgICAgICAgIE5vIG5ldyB0ZXN0 czogTm8gZmVhdHVyZSBhZGRpdGlvbnMvcmVtb3ZhbHMuCisKKyAgICAgICAgKiBhY2Nlc3NpYmls aXR5L0FjY2Vzc2liaWxpdHlJbWFnZU1hcExpbmsuY3BwOgorICAgICAgICAoV2ViQ29yZTo6QWNj ZXNzaWJpbGl0eUltYWdlTWFwTGluazo6QWNjZXNzaWJpbGl0eUltYWdlTWFwTGluayk6CisgICAg ICAgICogZWRpdGluZy9EZWxldGVTZWxlY3Rpb25Db21tYW5kLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkRlbGV0ZVNlbGVjdGlvbkNvbW1hbmQ6OkRlbGV0ZVNlbGVjdGlvbkNvbW1hbmQpOgorICAg ICAgICAqIGh0bWwvSFRNTENhbnZhc0VsZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SFRN TENhbnZhc0VsZW1lbnQ6OkhUTUxDYW52YXNFbGVtZW50KToKKyAgICAgICAgKiB4bWwvWFBhdGhS ZXN1bHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6WFBhdGhSZXN1bHQ6OlhQYXRoUmVzdWx0KToK KwogMjAxMS0wMy0xMSAgRGVhbiBKYWNrc29uICA8ZGlub0BhcHBsZS5jb20+CiAKICAgICAgICAg VW5yZXZpZXdlZC4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvQWNjZXNzaWJp bGl0eUltYWdlTWFwTGluay5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvYWNjZXNz aWJpbGl0eS9BY2Nlc3NpYmlsaXR5SW1hZ2VNYXBMaW5rLmNwcAkocmV2aXNpb24gODA4NTMpCisr KyBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlJbWFnZU1hcExpbmsu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC00Miw2ICs0Miw3IEBAIHVzaW5nIG5hbWVzcGFjZSBIVE1M TmFtZXM7CiBBY2Nlc3NpYmlsaXR5SW1hZ2VNYXBMaW5rOjpBY2Nlc3NpYmlsaXR5SW1hZ2VNYXBM aW5rKCkKICAgICA6IG1fYXJlYUVsZW1lbnQoMCkKICAgICAsIG1fbWFwRWxlbWVudCgwKQorICAg ICwgbV9wYXJlbnQoMCkKIHsKIH0KIApJbmRleDogU291cmNlL1dlYkNvcmUvZWRpdGluZy9EZWxl dGVTZWxlY3Rpb25Db21tYW5kLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9lZGl0 aW5nL0RlbGV0ZVNlbGVjdGlvbkNvbW1hbmQuY3BwCShyZXZpc2lvbiA4MDg1MykKKysrIFNvdXJj ZS9XZWJDb3JlL2VkaXRpbmcvRGVsZXRlU2VsZWN0aW9uQ29tbWFuZC5jcHAJKHdvcmtpbmcgY29w eSkKQEAgLTcyLDYgKzcyLDcgQEAgRGVsZXRlU2VsZWN0aW9uQ29tbWFuZDo6RGVsZXRlU2VsZWN0 aW9uQwogICAgICAgbV9oYXNTZWxlY3Rpb25Ub0RlbGV0ZShmYWxzZSksIAogICAgICAgbV9zbWFy dERlbGV0ZShzbWFydERlbGV0ZSksIAogICAgICAgbV9tZXJnZUJsb2Nrc0FmdGVyRGVsZXRlKG1l cmdlQmxvY2tzQWZ0ZXJEZWxldGUpLAorICAgICAgbV9uZWVkUGxhY2Vob2xkZXIoZmFsc2UpLAog ICAgICAgbV9yZXBsYWNlKHJlcGxhY2UpLAogICAgICAgbV9leHBhbmRGb3JTcGVjaWFsRWxlbWVu dHMoZXhwYW5kRm9yU3BlY2lhbEVsZW1lbnRzKSwKICAgICAgIG1fcHJ1bmVTdGFydEJsb2NrSWZO ZWNlc3NhcnkoZmFsc2UpLApAQCAtODgsNiArODksNyBAQCBEZWxldGVTZWxlY3Rpb25Db21tYW5k OjpEZWxldGVTZWxlY3Rpb25DCiAgICAgICBtX2hhc1NlbGVjdGlvblRvRGVsZXRlKHRydWUpLCAK ICAgICAgIG1fc21hcnREZWxldGUoc21hcnREZWxldGUpLCAKICAgICAgIG1fbWVyZ2VCbG9ja3NB ZnRlckRlbGV0ZShtZXJnZUJsb2Nrc0FmdGVyRGVsZXRlKSwKKyAgICAgIG1fbmVlZFBsYWNlaG9s ZGVyKGZhbHNlKSwKICAgICAgIG1fcmVwbGFjZShyZXBsYWNlKSwKICAgICAgIG1fZXhwYW5kRm9y U3BlY2lhbEVsZW1lbnRzKGV4cGFuZEZvclNwZWNpYWxFbGVtZW50cyksCiAgICAgICBtX3BydW5l U3RhcnRCbG9ja0lmTmVjZXNzYXJ5KGZhbHNlKSwKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2h0bWwv SFRNTENhbnZhc0VsZW1lbnQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwv SFRNTENhbnZhc0VsZW1lbnQuY3BwCShyZXZpc2lvbiA4MDg1MykKKysrIFNvdXJjZS9XZWJDb3Jl L2h0bWwvSFRNTENhbnZhc0VsZW1lbnQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC03Niw2ICs3Niw3 IEBAIHN0YXRpYyBjb25zdCBmbG9hdCBNYXhTa2lhRGltID0gMzI3NjcuMEYKIEhUTUxDYW52YXNF bGVtZW50OjpIVE1MQ2FudmFzRWxlbWVudChjb25zdCBRdWFsaWZpZWROYW1lJiB0YWdOYW1lLCBE b2N1bWVudCogZG9jdW1lbnQpCiAgICAgOiBIVE1MRWxlbWVudCh0YWdOYW1lLCBkb2N1bWVudCkK ICAgICAsIG1fc2l6ZShEZWZhdWx0V2lkdGgsIERlZmF1bHRIZWlnaHQpCisgICAgLCBtX3JlbmRl cmVySXNDYW52YXMoZmFsc2UpCiAgICAgLCBtX2lnbm9yZVJlc2V0KGZhbHNlKQogICAgICwgbV9w YWdlU2NhbGVGYWN0b3IoZG9jdW1lbnQtPmZyYW1lKCkgPyBkb2N1bWVudC0+ZnJhbWUoKS0+cGFn ZSgpLT5jaHJvbWUoKS0+c2NhbGVGYWN0b3IoKSA6IDEpCiAgICAgLCBtX29yaWdpbkNsZWFuKHRy dWUpCkluZGV4OiBTb3VyY2UvV2ViQ29yZS94bWwvWFBhdGhSZXN1bHQuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJDb3JlL3htbC9YUGF0aFJlc3VsdC5jcHAJKHJldmlzaW9uIDgwODUzKQor KysgU291cmNlL1dlYkNvcmUveG1sL1hQYXRoUmVzdWx0LmNwcAkod29ya2luZyBjb3B5KQpAQCAt NDEsNiArNDEsOCBAQCB1c2luZyBuYW1lc3BhY2UgWFBhdGg7CiAKIFhQYXRoUmVzdWx0OjpYUGF0 aFJlc3VsdChEb2N1bWVudCogZG9jdW1lbnQsIGNvbnN0IFZhbHVlJiB2YWx1ZSkKICAgICA6IG1f dmFsdWUodmFsdWUpCisgICAgLCBtX25vZGVTZXRQb3NpdGlvbigwKQorICAgICwgbV9kb21UcmVl VmVyc2lvbigwKQogewogICAgIHN3aXRjaCAobV92YWx1ZS50eXBlKCkpIHsKICAgICAgICAgY2Fz ZSBWYWx1ZTo6Qm9vbGVhblZhbHVlOgo=