Prevent crashes in mime_util_xdg.

Prevent crashes in mime_util_xdg.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152347

[benjhayden, 2012-08-15 21:15:04]

PTAL

[benjhayden, 2012-08-15 21:31:23]

For a little context, I ran into this crash while fixing another crash by
rewriting DownloadsDOMHandler. If DownloadsDOMHandler incorrectly tries to
generate an icon for a download with an empty filename, then mime_util_xdg will
crash with a mysterious message in third_party, without a backtrace.
I could add a DCHECK along with the short-circuits so we don't lose the
debugging signal if you like.

[brettw, 2012-08-16 03:37:33]

http://codereview.chromium.org/10831337/diff/1/base/nix/mime_util_xdg.cc
File base/nix/mime_util_xdg.cc (right):

http://codereview.chromium.org/10831337/diff/1/base/nix/mime_util_xdg.cc#newc...
base/nix/mime_util_xdg.cc:591: if (filepath.empty()) return "";
Don't use single-line style except in unusual cases. Also, return std::string()
is more efficient and is semantically correct, otherwise it calls std::string's
constructor with an empty string which it has to interpret.

http://codereview.chromium.org/10831337/diff/1/base/nix/mime_util_xdg.cc#newc...
base/nix/mime_util_xdg.cc:630: icon_name = "";
Can't this be removed and you put the else case in an if (!mime_type.empty())?

[benjhayden, 2012-08-16 14:33:34]

PTAL

http://codereview.chromium.org/10831337/diff/1/base/nix/mime_util_xdg.cc
File base/nix/mime_util_xdg.cc (right):

http://codereview.chromium.org/10831337/diff/1/base/nix/mime_util_xdg.cc#newc...
base/nix/mime_util_xdg.cc:591: if (filepath.empty()) return "";
On 2012/08/16 03:37:34, brettw wrote:
> Don't use single-line style except in unusual cases. Also, return
std::string()
> is more efficient and is semantically correct, otherwise it calls
std::string's
> constructor with an empty string which it has to interpret.

Done.

http://codereview.chromium.org/10831337/diff/1/base/nix/mime_util_xdg.cc#newc...
base/nix/mime_util_xdg.cc:630: icon_name = "";
On 2012/08/16 03:37:34, brettw wrote:
> Can't this be removed and you put the else case in an if (!mime_type.empty())?

Done.

[brettw, 2012-08-19 22:30:54]

lgtm

[commit-bot: I haz the power, 2012-08-20 15:36:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10831337/5002

[commit-bot: I haz the power, 2012-08-20 17:15:47]

Change committed as 152347

[Lei Zhang, 2012-08-22 05:59:10]

https://chromiumcodereview.appspot.com/10831337/diff/5002/base/nix/mime_util_...
File base/nix/mime_util_xdg.cc (right):

https://chromiumcodereview.appspot.com/10831337/diff/5002/base/nix/mime_util_...
base/nix/mime_util_xdg.cc:591: if (filepath.empty())
Curious, who is calling GetFileMimeType with an empty filepath?
IconManager::LoadIcon() ?

[benjhayden, 2012-08-22 14:24:14]

https://chromiumcodereview.appspot.com/10831337/diff/5002/base/nix/mime_util_...
File base/nix/mime_util_xdg.cc (right):

https://chromiumcodereview.appspot.com/10831337/diff/5002/base/nix/mime_util_...
base/nix/mime_util_xdg.cc:591: if (filepath.empty())
On 2012/08/22 05:59:10, Lei Zhang wrote:
> Curious, who is calling GetFileMimeType with an empty filepath?
> IconManager::LoadIcon() ?

Yes, LoadIcon() doesn't early-return for empty paths, iirc.
No, nothing is currently calling LoadIcon() with an empty path, afaik. We're
currently refactoring the downloads system, and in a few of my recent refactors,
I had bugs where I was calling LoadIcon() with an empty path. I fixed the bugs,
but until I get around to writing a lot more tests, it could easily come back,
and I'd rather display a blank icon than crash.

[Lei Zhang, 2012-08-22 18:43:20]

On 2012/08/22 14:24:14, benjhayden_chromium wrote:
>
https://chromiumcodereview.appspot.com/10831337/diff/5002/base/nix/mime_util_...
> File base/nix/mime_util_xdg.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10831337/diff/5002/base/nix/mime_util_...
> base/nix/mime_util_xdg.cc:591: if (filepath.empty())
> On 2012/08/22 05:59:10, Lei Zhang wrote:
> > Curious, who is calling GetFileMimeType with an empty filepath?
> > IconManager::LoadIcon() ?
> 
> Yes, LoadIcon() doesn't early-return for empty paths, iirc.
> No, nothing is currently calling LoadIcon() with an empty path, afaik. We're
> currently refactoring the downloads system, and in a few of my recent
refactors,
> I had bugs where I was calling LoadIcon() with an empty path. I fixed the
bugs,
> but until I get around to writing a lot more tests, it could easily come back,
> and I'd rather display a blank icon than crash.

I would suggest adding a NOTREACHED() there, so developers know its not suppose
to happen, rather than letting the bug go by silently.