Closed Bug 620305 Opened 9 years ago Closed 9 years ago

crash [@ nsNavHistory::GetStringFromName] when GetBundle() fails


(Toolkit :: Places, defect, critical)

Not set





(Reporter: timeless, Assigned: timeless)


(Blocks 1 open bug)


(Keywords: coverity, crash)

Crash Data


(1 file, 1 obsolete file)

7149 nsNavHistory::GetStringFromName(const PRUnichar *aName, nsACString& aResult)

7151   nsIStringBundle *bundle = GetBundle();

this tries to handle !bundle:
7152   if (!bundle)
7153     aResult.Truncate(0);

and then immediately crashes:
7156   nsresult rv = bundle->GetStringFromName(aName, getter_Copies(value));

it should return after 7153 as part of a block
Version: unspecified → Trunk
Attached patch proposal (obsolete) — Splinter Review
Assignee: nobody → timeless
Attachment #498694 - Flags: review?(mak77)
Attachment #498694 - Flags: approval2.0?
Comment on attachment 498694 [details] [diff] [review]

is there a specific reason you use the name as string instead of just truncating? Just to provide something more useful?
having a ui that collapses to nothing is almost certainly less helpful than having a ui which users can google for.
Maybe, but in the other error cases (the bundle exists but the string does not, or in in GetAgeInDaysString) we instead return a empty string.
JS calls won't do anything different than throwing and collapse ui. But here we can't throw since we don't return a rv.
At least I think the code should be consistent, so if we are going to return the name here, we should return it in all error cases in GetStringFromName and GetAgeInDaysString.
fwiw i'm using []s as a fallback for month, using ()s would be a bad idea since ()s tend to indicate count.

The code currently *pastes* month with year (bug 620627), so it's possible to get "January 2000" or as fallback "[1] 2000". I don't think using some variation of 'Mon' would be a good idea since Mon could be confused for Monday instead of Month, and I don't think there are any internationally recognized formats for months.
Attachment #498694 - Attachment is obsolete: true
Attachment #498972 - Flags: review?(mak77)
Attachment #498972 - Flags: approval2.0?
Attachment #498694 - Flags: review?(mak77)
Attachment #498694 - Flags: approval2.0?
Comment on attachment 498972 [details] [diff] [review]
provide fallback for each case

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+  aResult = nsPrintfCString("[%d]", aIndex);

Probably "M - YYYY" or "M / YYYY" are more globally recognized entries even if not a standard. I'd go for one of these, but I don't have strong preferences, this code path is touched only in case of bad errors, and I'd guess we would have major problems before hitting it.

Just in case, there should be a couple tests called test_sidebar_something or test_history_sidebar... try to run them or do a try run on linux xpcshell.
Attachment #498972 - Flags: review?(mak77) → review+
Comment on attachment 498972 [details] [diff] [review]
provide fallback for each case

So I spoke w/ mak about the []s, there's no useful context available (we don't know here if there's going to be a year), and since it's an error case it should never really happen, having some marker will make it easier for people to ask about it.

mak asked me to ask him to run the xpcshell tests as I'm not in a great position to do that.
Attachment #498972 - Flags: feedback?(mak77)
Comment on attachment 498972 [details] [diff] [review]
provide fallback for each case

ok, I've run xpcshell tests and they pass.
Attachment #498972 - Flags: feedback?(mak77)
Attachment #498972 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
Crash Signature: [@ nsNavHistory::GetStringFromName]
You need to log in before you can comment on or make changes to this bug.