493602 - UMR when clicking history menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Robert Sayre]

==58681== Conditional jump or move depends on uninitialised value(s)
==58681==    at 0x613AA: dosprintf (in /Users/sayrer/dev/clean-debug-tracemonkey/nsprpub/pr/src/libnspr4.dylib)
==58681==    by 0x61D94: PR_vsnprintf (in /Users/sayrer/dev/clean-debug-tracemonkey/nsprpub/pr/src/libnspr4.dylib)
==58681==    by 0x3D44D85: nsPrintfCString::nsPrintfCString(unsigned int, char const*, ...) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x3B4789A: nsNavHistory::ConstructQueryString(nsCOMArray<nsNavHistoryQuery> const&, nsNavHistoryQueryOptions*, nsCString&, int&, nsDataHashtable<nsCStringHashKey, nsCString>&) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x3B4B67E: nsNavHistory::GetQueryResults(nsNavHistoryQueryResultNode*, nsCOMArray<nsNavHistoryQuery> const&, nsNavHistoryQueryOptions*, nsCOMArray<nsNavHistoryResultNode>*) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x3B6F406: nsNavHistoryQueryResultNode::FillChildren() (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x3B7061C: nsNavHistoryQueryResultNode::OpenContainer() (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x3B690BA: nsNavHistoryContainerResultNode::SetContainerOpen(int) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x3B76489: nsNavHistoryQueryResultNode::SetContainerOpen(int) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x3D2D070: NS_InvokeByIndex_P (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x2CC4343: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==    by 0x2CD3AA6: XPCWrappedNative::SetAttribute(XPCCallContext&) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)
==58681==  Uninitialised value was created by a stack allocation
==58681==    at 0x3B471E0: nsNavHistory::ConstructQueryString(nsCOMArray<nsNavHistoryQuery> const&, nsNavHistoryQueryOptions*, nsCString&, int&, nsDataHashtable<nsCStringHashKey, nsCString>&) (in /Users/sayrer/dev/clean-debug-tracemonkey/toolkit/library/XUL)

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Looks like something we should fix, D?

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> Looks like something we should fix, D?
Yes, yes we should.  We should probably block on it in fact.

Comment 3

[Marco Bonardo [:mak]]

tentatively assigning to Shawn, reassign if you can't look at it.

Comment 4

[Shawn Wilsher :sdwilsh]

So, this is complaining that something in ConstructQueryString is allocated on the stack, and not being initialized, and then being passed to nsPrintfCString, right?  If so, I think this may be bogus.  All calls to nsPrintfCString use literal strings and numerical constants in this method, so I don't so how we could have anything uninitialized.

Comment 5

[Mike Shaver (:shaver emeritus)]

Copying nick -- I'm not sure what it's telling us either, tbh.

Comment 6

[Nicholas Nethercote [inactive]]

I looked at the code but was unable to see what the problem could be.  However, Valgrind's undefined value warnings are generally very reliable.  And Valgrind does analyze at the binary level, not source code level, so it's conceivable something is wrong even though the C++ looks ok.

I suggest trying to hunt down the problem with the VALGRIND_CHECK_MEM_IS_DEFINED or VALGRIND_CHECK_VALUE_IS_DEFINED client requests.  It may be useful to put some of the arguments to nsPrintfCString() in temporary variables in order to check them.  This is a bit laborious, as it requires recompiling, but it's the best way to investigate further.

Comment 7

[Shawn Wilsher :sdwilsh]

With line numbers:
==22136== Conditional jump or move depends on uninitialised value(s)
==22136==    at 0x4FFBFF1: dosprintf (in /home/sdwilsh/central/obj-i686-pc-linux-gnu/nsprpub/pr/src/libnspr4.so)
==22136==    by 0x4FFCCD0: PR_vsnprintf (in /home/sdwilsh/central/obj-i686-pc-linux-gnu/nsprpub/pr/src/libnspr4.so)
==22136==    by 0x4BC8F7E: nsPrintfCString::nsPrintfCString(unsigned, char const*, ...) (nsPrintfCString.cpp:81)
==22136==    by 0x4A699EA: nsNavHistory::ConstructQueryString(nsCOMArray<nsNavHistoryQuery> const&, nsNavHistoryQueryOptions*, nsCString&, int&, nsDataHashtable<nsCStringHashKey, nsCString>&) (nsNavHistory.cpp:4088)
==22136==    by 0x4A69AD3: nsNavHistory::GetQueryResults(nsNavHistoryQueryResultNode*, nsCOMArray<nsNavHistoryQuery> const&, nsNavHistoryQueryOptions*, nsCOMArray<nsNavHistoryResultNode>*) (nsNavHistory.cpp:4171)
==22136==    by 0x4A7C09C: nsNavHistoryQueryResultNode::FillChildren() (nsNavHistoryResult.cpp:2370)
==22136==    by 0x4A7C797: nsNavHistoryQueryResultNode::OpenContainer() (nsNavHistoryResult.cpp:2153)
==22136==    by 0x4A77E6B: nsNavHistoryContainerResultNode::SetContainerOpen(int) (nsNavHistoryResult.cpp:449)
==22136==    by 0x4BBDA72: NS_InvokeByIndex_P (in /home/sdwilsh/central/obj-i686-pc-linux-gnu/toolkit/library/libxul.so)
==22136==    by 0x424CDFA: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xpcwrappednative.cpp:2474)
==22136==    by 0x4255713: XPCWrappedNative::SetAttribute(XPCCallContext&) (xpcprivate.h:2318)
==22136==    by 0x4254E42: XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned, int*, int*) (xpcwrappednativejsops.cpp:1614)

Comment 8

[Shawn Wilsher :sdwilsh]

Oh, I think I see what this is.  I don't think we are passing enough arguments here...

Comment 9

[Shawn Wilsher :sdwilsh]

yup, this is a regression from bug 487777.  Fix RSN.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 378643 [details] [diff] [review]
v1.0

i guess if we could file a bug to make so that nsPrintfCString does not compile or warns in these cases. At least in debug mode.

Comment 12

[Shawn Wilsher :sdwilsh]

I filed bug 493996 for a static analysis.

Comment 13

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/f011cb3929c7

Comment 14

[Shawn Wilsher :sdwilsh]

So, this doesn't actually exist on branch, so I'm removing the blocking bits...

Comment 15

[Nicholas Nethercote [inactive]]

Could GCC's 'format' attribute be used?  It's used once in js/src/jsregexp.cpp, on re_debug().

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Could GCC's 'format' attribute be used?  It's used once in js/src/jsregexp.cpp,
> on re_debug().
See the spinoff bug 493996.

Comment 17

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv