416743 - [gtk icon decoder] "ASSERTION: aFileExt shouldn't start with a dot" loading directory listing

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

When I load the directory listing of layout/reftests/bugs/ in my source tree, I see a few assertions of the form "ASSERTION: aFileExt shouldn't start with a dot".  This seems to happen because nsIconChannel::InitWithGnome does the following:
401           nsCAutoString fileExt;
402           aIconURI->GetFileExtension(fileExt);
403           ms->GetTypeFromExtension(fileExt, type);
nsIconURI::GetFileExtension includes a dot (explicitly in the implementation, although not documented in nsIIconURI).  nsIMIMEService::GetTypeFromExtension is documented as requiring that the dot not be present.  It seems this mistake has been around forever; I'm not sure why it just started happening.

#1  0x00002aaaab350e5b in NS_DebugBreak_P (aSeverity=1, 
    aStr=0x2aaab2fd9d28 "aFileExt shouldn't start with a dot", 
    aExpr=0x2aaab2fd9d14 "aFileExt[0] != '.'", 
    aFile=0x2aaab2fd9c60 "/home/dbaron/builds/trunk/mozilla/uriloader/exthandler/unix/nsGNOMERegistry.cpp", aLine=118)
    at /home/dbaron/builds/trunk/mozilla/xpcom/base/nsDebugImpl.cpp:358
#2  0x00002aaab2fb434f in nsGNOMERegistry::GetFromExtension (
    aFileExt=@0x7fff61587a90)
    at /home/dbaron/builds/trunk/mozilla/uriloader/exthandler/unix/nsGNOMERegistry.cpp:118
#3  0x00002aaab2fb1df6 in nsOSHelperAppService::GetFromExtension (
    this=0x6784c0, aFileExt=@0x7fff61587a90)
    at /home/dbaron/builds/trunk/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp:1363
#4  0x00002aaab2fb2520 in nsOSHelperAppService::GetMIMEInfoFromOS (
    this=0x6784c0, aType=@0x2aaaab5dee80, aFileExt=@0x7fff61587cf0, 
    aFound=0x7fff61587b7c)
    at /home/dbaron/builds/trunk/mozilla/uriloader/exthandler/unix/nsOSHelperApp---Type <return> to continue, or q <return> to quit---
Service.cpp:1599
#5  0x00002aaab2f9c61f in nsExternalHelperAppService::GetTypeFromExtension (
    this=0x6784c0, aFileExt=@0x7fff61587cf0, aContentType=@0x7fff61587dd0)
    at /home/dbaron/builds/trunk/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp:2434
#6  0x00002aaabdd66eeb in nsIconChannel::InitWithGnome (this=0x1825ac0, 
    aIconURI=0x186d700)
    at /home/dbaron/builds/trunk/mozilla/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp:403

Comment 1

[Mike Hommey [:glandium]]

It obviously started happening after the landing of bug #373397.

Now the question is whether nsIconURI::GetFileExtensions should be changed to not include the dot, or the dot removed from there before calling nsIMIMEService::GetTypeFromExtension.

Comment 2

[Martin Mokrejs]

Attachment: stacktrace

I get this with currnt cvs HEAD build of suite. While double-clicking on an attachment to an email message ooffice should get opened, actually the download manager comes up first. I got the assertion. Probably I have pressed 'c' as contine on the first assert the oofice started. At that moment I acquired the stacktraces, so maybe little late? But from gdb output it seems to me I was still at the same breakpoint. Anyway, judge yourself.

Comment 3

[Mike Hommey [:glandium]]

This is basically the same stacktrace as dbaron's.

Comment 4

[Mike Hommey [:glandium]]

Actually, from a quick glance, it looks like only modules/libjar/nsJARChannel.cpp does the right thing and strips the . from the extension. Wouldn't it be better to fit reality and change nsIMIMEService::GetTypeFromExtension documentation and code instead of callers ?

Comment 5

[Boris Zbarsky [:bzbarsky]]

nsExternalHelperAppService::GetTypeFromURI does the right thing.  So does anyone getting the extension from an nsIFile.  The mimemoz2.cpp code does the right thing too.  The binhex decoder seems to be the only other caller that screws up, no?

I wouldn't object to making the leading '.' optional, I guess.  But then it neds to be done.  And there is some risk, esp. at this stage in the game, since it's a behavior change for an API various consumers use.

Comment 6

[Mike Hommey [:glandium]]

So, what about stripping the leading '.' in nsGNOMERegistry::GetFromExtension, remove the assertion, but add a debug message when the extension does *not* contain a dot, so that such cases can be spotted when it will be time to fix the API?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I would actually prefer to push the assertion up to nsExternalHelperAppService::GetFromExtension as long as the API remains as it is.

Comment 8

[Mike Hommey [:glandium]]

But what about the present bug, then ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Either the the API consumer should be fixed, or the API should be changed.  Or nothing should happen, leaving the bug unfixed.  I don't know which is riskier at this point.

Comment 10

[Martin Mokrejs]

Attachment: gdb stacktrace

I am also hitting this while trying to open directiory in a browser by specifying URL like file:///home/mmokrejs/public_html/tmp . I guess because it tries to load some icons for the files present. Current cvs HEAD build.

Comment 13

[Martin Mokrejs]

This still happens for me with 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.3pre) Gecko/20090801 SeaMonkey/2.0b2pre

when I scroll in the download manager window up or down with the mouse wheel.

Comment 15

[Jeff Walden [:Waldo]]

Bug 525054 comment 1 has sufficient detail for someone reasonably new to Mozilla code to fix it.

I tend to think we shouldn't put a leading dot in nsIIconURI.fileExtension, because it seems many consumers will just want to strip it and because it's not how sibling methods like nsIURL.fileExtension work (although, as noted there, nsIURL.fileExtension doesn't quite explicitly say this, but nsStandardURL strips, and that's the gold standard at the moment).  The comment I mention above suggests this fix, as well.

Comment 16

[Brian Crowder]

Attachment: kill this assertion

The assertion is bogus, at least for now.  The extra leading "." doesn't cause badness afaict.  We can restore it if/when that statement becomes untrue.  In the meantime, there is resistance to fixing/changing the API, so let's just do this?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 469959 [details] [diff] [review]
kill this assertion

OK.

Comment 18

[Brian Crowder]

Attachment: with a commit-comment, land this one, please.

Comment 19

[Daniel Holbert [:dholbert]]

pushed: http://hg.mozilla.org/mozilla-central/rev/cd9a856c3df4