415273 - moz-icon failing to display correct icon with contentType parameter specified

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Jim Mathies [:jimm]]

Recently moz-icon urls that specify contentType as a parameter fail to display the correct icon for certain application file types. The icon displayed is the generic "white paper" icon instead.

To reproduce, type - 
 
"moz-icon://somefile.zip?size=32&contentType=application/zip"

in the address bar.

Result: you should see the generic icon.

Display is correct if you remove the contentType parameter - 

"moz-icon://somefile.zip?size=32"

A found at least one other app that failed as well -
 
moz-icon://somefile.ipa?size=32&contentType=application/x-itunes-ipa
vs.
moz-icon://somefile.ipa?size=32
which worked.

This shows up most notably in the content handler dialog and in the app picker dialog when users click on a link to a zip file.

A little debugging - the icon file is opened successfully in both cases within  nsIconCahnnel - 
http://lxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp#273

Comment 1

[Jim Mathies [:jimm]]

Attachment: example shot

Comment 2

[Jim Mathies [:jimm]]

Attachment: working shot

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This should be a pretty easy fix.

Comment 4

[Stuart Parmenter]

Jim: If you can put a patch together for this request approval

Comment 5

[Jim Mathies [:jimm]]

working it now...

Comment 6

[Jim Mathies [:jimm]]

Attachment: nsIconChannel patch v.1

Comment 7

[[e-mail]]

Comment on attachment 310534 [details] [diff] [review]
nsIconChannel patch v.1

Nice. As a bonus, this will make it easier to convert the code to Unicode APIs.

>+    mimeService->GetPrimaryExtension(contentType, fileExt, defFileExt);
I wonder whether we're allowed to reuse the in string as the out string?

Comment 8

[Jim Mathies [:jimm]]

> I wonder whether we're allowed to reuse the in string as the out string?

Didn't seem to cause any problems when I was testing it.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310534 [details] [diff] [review]
nsIconChannel patch v.1

Tests?

Comment 10

[[e-mail]]

(In reply to comment #7)
> >+    mimeService->GetPrimaryExtension(contentType, fileExt, defFileExt);
> I wonder whether we're allowed to reuse the in string as the out string?

it's an implementation detail that it works - in this case, it will. relying on that is probably a bad idea, though, unless you're calling from within the implementation itself.

Comment 11

[Mike Schroepfer]

Jim - thoughts on comment 10?

Comment 12

[Jim Mathies [:jimm]]

I'll go ahead and roll together a new patch to address this.

I've also been looking for ways to add tests as well but haven't come up with anything. The bug is related to application associations which obviously change from machine to machine. I was looking for a way to create an icon compare test but haven't found anything i'd consider reliable.

Comment 13

[Jim Mathies [:jimm]]

Attachment: nsIconChannel patch v.2

updated to force a string copy in GetPrimaryExtension.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #13)
> updated to force a string copy in GetPrimaryExtension.

Why?

Comment 15

[Jim Mathies [:jimm]]

Did I somehow misinterpret the concern that was raised? I thought the issue was with the in/out behavior of the extension string.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

nsMIMEInfoBase::GetPrimaryExtension has no inout parameter... do you mean nsExternalHelperAppService::GetPrimaryExtension?

(but I think that should be fixed in the caller)

Comment 17

[[e-mail]]

(In reply to comment #15)
> Did I somehow misinterpret the concern that was raised? I thought the issue was
> with the in/out behavior of the extension string.
I'm sorry if I confused you; I was wondering why you had used two different strings, and dwitte replied confirming that your patch v.1 was correct.

Comment 18

[Jim Mathies [:jimm]]

Ok, I guess I should have had a little more faith. The extension string passes into GetPrimaryExtension as fileExt, and then back out as defFileExt. It's shared so it's just the same string. I didn't see any problems with that but strings in mozilla are still a little hazy for me so I assumed I had screwed up due somehow.

If everybody is cool with the first patch, I'll mark it as checkin-needed.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 310534 [details] [diff] [review]
nsIconChannel patch v.1

actually... one more to go.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310534 [details] [diff] [review]
nsIconChannel patch v.1

a1.9=beltzner

Comment 21

[Reed Loden [:reed]]

Checking in modules/libpr0n/decoders/icon/win/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp,v  <--  nsIconChannel.cpp
new revision: 1.46; previous revision: 1.45
done

Comment 22

[pal-moz]

cause bug 427978 ?