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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[timeless]

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

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 498694 [details] [diff] [review]
proposal

is there a specific reason you use the name as string instead of just truncating? Just to provide something more useful?

Comment 3

[timeless]

having a ui that collapses to nothing is almost certainly less helpful than having a ui which users can google for.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[timeless]

Attachment: provide fallback for each case

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.

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[timeless]

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.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 498972 [details] [diff] [review]
provide fallback for each case

ok, I've run xpcshell tests and they pass.

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/0600f6c6e35a