236138 - Bulletproof startup against a silly lack of preferences

Status: RESOLVED FIXED

(Core Graveyard :: Cmd-line Features, defect)

Description

[Benjamin Smedberg]

I got my build into a situation where the chrome://browser-locale/locale/ chrome
package wasn't registered properly, and instead of failing gracefully, the app
crashed because the browser.startup.homepage pref wasn't present. This is a
little patch to bulletproof against that situation.

Comment 1

[Benjamin Smedberg]

Attachment: bulletproof missing browser.startup.homepage pref

Comment 2

[Darin Fisher]

Comment on attachment 142700 [details] [diff] [review]
bulletproof missing browser.startup.homepage pref

> NS_IMETHODIMP nsBrowserContentHandler::GetDefaultArgs(PRUnichar **aDefaultArgs)
...
>+  nsXPIDLString args;
...
>   if (NeedHomepageOverride(prefs)) {
>     nsXPIDLString url;
>     rv = prefs->GetLocalizedUnicharPref(PREF_HOMEPAGE_OVERRIDE_URL, getter_Copies(url));
>     if (NS_SUCCEEDED(rv) && (const PRUnichar *)url) {
>       args = url;
>     }

it looks like you could avoid this copy by just using args directly in the
getter_Copies above.  in fact, there doesn't even seem to be much point to
inspecting |rv| if you assume that GetLocalizedUnicharPref will return an
empty string whenever it returns NS_FAILED(rv).


>+  if (!args.get()) {

i think |args.IsEmpty()| is better.


>         case 1: {
>           // skip the code below
>+          rv = GetHomePageGroup(prefs, getter_Copies(args));
>+          break;

why assign a value to |rv|?  it is never referenced down below.  hmm... looks
like
the other cases have the same problem.	care to clean those up as well?


r+sr=darin with those changes.


can this situation happen to a normal user?  or were you doing something
unusual?
do we need to worry about older releases?  is this a candidate for a 1.6.1 if
that
ever happens?

Comment 3

[Benjamin Smedberg]

Attachment: do-over, with some extra code from Neil

Neil made some good suggestions in bug 236208 which intersect with this, so I
just integrated them... we can use early returns to drop the xpidlstring
altogether.

Comment 4

[Benjamin Smedberg]

Comment on attachment 142756 [details] [diff] [review]
do-over, with some extra code from Neil

darin, could you just look over this again to make sure I didn't do anything
stupid?

Comment 5

[Darin Fisher]

Comment on attachment 142756 [details] [diff] [review]
do-over, with some extra code from Neil

>Index: nsBrowserInstance.cpp

>+              *aDefaultArgs = ToNewUnicode(NS_ConvertUTF8toUTF16(curl));

this is what "UTF8ToNewUnicode" is for :-)

sr=darin with that change.

Comment 6

[Benjamin Smedberg]

fixed on trunk

Neil, I incorporated your changes, so all you need to do in bug 236208 is change
cargs from nsXPIDLCString to nsCString and remove the getter_Copies.