625119 - When displaying moz-icon://unknown: ###!!! ASSERTION: Empty aExtension parameter!: '!aExtension.IsEmpty()', file nsExternalHelperAppService.cpp, line 2918

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Daniel Holbert [:dholbert]]

When I load either of these URLs in a debug mozilla-central build...
   ftp://ftp.mozilla.org/
   moz-icon://unknown?size=16
 (the former URL uses the latter as the icon for "README")
...I get this assertion-failure:
###!!! ASSERTION: Empty aExtension parameter!: '!aExtension.IsEmpty()', file ../../../mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp, line 2918

This is on Linux (Ubuntu 10.10) with an up-to-date mozilla-central debug build (at revision http://hg.mozilla.org/mozilla-central/rev/ae4ee53297b0 )

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 503306 [details] [diff] [review]
fix

Just in the way of patch-explanation...

>   if (type.IsEmpty()) {
>     nsCOMPtr<nsIMIMEService> ms(do_GetService("@mozilla.org/mime;1"));
>     if (ms) {
>       nsCAutoString fileExt;
>       aIconURI->GetFileExtension(fileExt);
>-      ms->GetTypeFromExtension(fileExt, type);
>+      if (!fileExt.IsEmpty()) {
>+        ms->GetTypeFromExtension(fileExt, type);
>+      }

In this case, aIconURI is "moz-icon://unknown", and GetFileExtension returns an empty string.

I humbly submit that we should skip the "GetTypeFromExtension" call for empty-type (which is what this patch does).

It seems there's no point in calling it -- the documentation for that method says:
> 76      * Retrieves a ACString representation of the MIME type
> 77      * associated with this file extension.
http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsIMIMEService.idl#76
and while I don't claim to be a MIME-type expert, I doubt (?) that we have a useful mimetype associated with the extension "". (and this bug's failing assertion seems to back me up in that view)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> I humbly submit that we should skip the "GetTypeFromExtension" call for
> empty-type (which is what this patch does).

sorry, s/empty-type/empty-fileExt/

(though |type| does happen to be empty, too, as evidenced by the fact that we're inside of a "if type.IsEmpty()" clause.)

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 503306 [details] [diff] [review]
fix

What happens if _gnome_icon_lookup gets a null/empty string for its mimetype?

I suppose that's already happening anyways...

Comment 5

[Daniel Holbert [:dholbert]]

> What happens if _gnome_icon_lookup gets a null/empty string for its mimetype?

(mimetype is empty string in this case, not null)

I just checked in gdb -- it returns "gnome-fs-regular".

> I suppose that's already happening anyways...

Correct, if no plugins are installed.

However, if you have plugin content-handlers installed (e.g. VLC), something else happens, interestingly enough -- the plugin fills in its own mimetype inside of the "ms->GetTypeFromExtension(fileExt, type)" call. So, since I have VLC plugin installed, I end up with type = "application/x-vlc-plugin".

Apparently this is because, inside of GetTypeFromExtension, the line...
>    if (NS_SUCCEEDED(pluginHost->IsPluginEnabledForExtension(flatExt.get(), mimeType))) {
...succeeds (where |flatExt| = "" and |mimeType| = "application/x-vlc-plugin")

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2756

However, this all doesn't really matter, because _gnome_icon_lookup still ends up returning the same thing ("gnome-fs-regular") regardless of whether |type| is empty or contains the string "application/x-vlc-plugin".

Comment 6

[Daniel Holbert [:dholbert]]

This isn't particularly spammy, so there's no real reason to land it soon.

Flagging to land post-Firefox-4.

Comment 7

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/fc1935dc3987