Closed Bug 620305 Opened 9 years ago Closed 9 years ago
crash [@ ns
Nav History::Get String From Name] when Get Bundle() fails
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 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?
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 " 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 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.
Comment on attachment 498972 [details] [diff] [review] provide fallback for each case ok, I've run xpcshell tests and they pass.
Attachment #498972 - Flags: approval2.0? → approval2.0+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
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.