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

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: dbaron, Assigned: Brian Crowder)

Tracking

({assertion})

Trunk
x86
Linux
assertion
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
Keywords: assertion
Version: unspecified → Trunk
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

10 years ago
Created attachment 311942 [details]
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.
This is basically the same stacktrace as dbaron's.
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 ?
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.
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?
I would actually prefer to push the assertion up to nsExternalHelperAppService::GetFromExtension as long as the API remains as it is.
But what about the present bug, then ?
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

10 years ago
Created attachment 315837 [details]
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.
Duplicate of this bug: 450409
Duplicate of this bug: 451395

Comment 13

8 years ago
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.
Duplicate of this bug: 525054
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.
Whiteboard: [good first bug]
(Assignee)

Comment 16

7 years ago
Created attachment 469959 [details] [diff] [review]
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?
Assignee: nobody → crowderbt
(Assignee)

Updated

7 years ago
Attachment #469959 - Flags: review?(bzbarsky)
Comment on attachment 469959 [details] [diff] [review]
kill this assertion

OK.
Attachment #469959 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 18

7 years ago
Created attachment 469986 [details] [diff] [review]
with a commit-comment, land this one, please.
Attachment #469959 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
pushed: http://hg.mozilla.org/mozilla-central/rev/cd9a856c3df4
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.