675356 - Dmg files sometimes not handled correctly

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Instead I get the following message:

The application you chose ("") could not be found.  Check the file name or choose another application.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

FWIW, I've always gotten this behavior and thought it was just my machine until a clean OS X install today.

Comment 2

[Alex Keybl [:akeybl]]

Attachment: Proposed patch for 675356

Comment 3

[Alex Keybl [:akeybl]]

From my read of uriloader/exthandler/mac/nsOSHelperAppService.mm's GetMIMEInfoFromOS, LSCopyApplicationForMIMEType is not offering

"/System/Library/CoreServices/DiskImageMounter.app"

for the "application/x-apple-diskimage" type, yet LSGetApplicationForInfo does return that app for the ".dmg" extension. We fall into an unhandled situation where the MIME info is populated, the MIME type is not mapped to anything (Apple bug?), and the extension offers the correct app. It should be properly handled in the proposed patch, which now defaults to the extension's associated helper app in this case.

I'll figure out how to get this on the try server soon.

Comment 4

[Alex Keybl [:akeybl]]

I don't think my fix is the right one according to spec (although it seems to do the job). That being said, I do believe my analysis is correct. Resetting the assignee to default and the status to new so somebody else can take a stab.

Comment 5

[Robert Strong (they/them - no direct email)]

Steven, when you have a moment please take a look at this in the hope that it is a fairly straight forward easy to fix bug?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

OK, Robert.

> Opening dmg files directly from the download prompt no longer works

But first someone needs to tell me precisely what this means.  In other words, give me steps to reproduce.

Also tell me what versions of FF it occurs with (if FF is involved at all), and on what versions of OS X.

Comment 7

[Alex Keybl [:akeybl]]

Not sure about regression range, but I just reproduced this bug on FF9

1) Go to http://growl.info/downloads
2) Click "Growl 1.2.2" - http://growl.googlecode.com/files/Growl-1.2.2.dmg
3) Notice "Open With" is blank, when it should be something like DiskImageMounter
4) Click "Open With"
5) Click "OK"
6) Get dialog that "Application could not be found"

Expected:
"Open With" opens the DMG once downloaded, by default, without selecting "/System/Library/CoreServices/DiskImageMounter.app".

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Thanks, Alex.  This bug now makes some sense :-)

Comment 9

[Steven Michaud [:smichaud] (Retired)]

I've started looking into this, and have discovered that it's partly
an Apple bug -- DiskImageMounter isn't registered to handle any
mimetypes at all!

I found this out using a wonderful (and very old) little app (actually
a preference pane) called Default Apps:
http://www.rubicode.com/Software/RCDefaultApp/

Alex already discovered	at least part of this, when he noticed (in
comment #3 that	nothing	is registered to handle	the
"application/x-apple-diskimage"	mimetype.  But it's much easier	to
debug this kind of thing using Default Apps.

DiskImageMounter *is* registered to handle the "dmg" extension, plus
the "com.apple.disk-image-udif" UTI and the "devi" "file type".

I'll look through our Mac "OS Helper" code to see if I can find a
reasonable way to deal with a download whose mimetype isn't handled
but whose extension is handled.

By the way, this bug goes back to FF 3.6 days (FF 3.6 has it, but FF
3.5 doesn't), and presumably to when Josh and I rewrote our "OS
Helper" code to stop using Apple's ancient (pre-Carbon) Internet
Config service (bug 489864).

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Attachment: Possible fix

This is definitely an Apple bug -- or (as my patch comment says) an
Apple design flaw.  And a strict interpretation of the spec would make
it impossible for us to do anything about it (the doc on
nsExternalHelperAppService::GetMIMEInfoFromOS()	says that aMIMEType
should be given	precedence over	aFileExt).

But given a proper understanding of what the design flaw actually is,
I think we can stretch the rules without breaking them ... and
hopefully without triggering other bugs.

The basic problem is as follows:  If an Apple app is registered to
handle a given UTI, it isn't necessarily also registered to handle the
corresponding MIME type (or types).  So if the only app capable of
handling a given MIME time is an Apple app, Launch Services doesn't
always find it.

This doesn't seem to be true for extensions:  Launch Services appears
to do a better job of finding Apple apps that are capable of handling
files with such-and-such an extension.

So to work around Apple's design flaw we need to reliably identify
cases where an app's been installed that should have been registered
to handle a given MIME type, even though it wasn't.

I think my patch should do this reasonably well. Given an app that's
been registered	to handle a particular extension, and a	MIME type for
which no app has been registered, it checks whether the	app might have
failed to be registered	because	of Apple's design flaw.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 581074 [details] [diff] [review]
Possible fix

Josh, please let us know if you don't have time to review this.  In that case I'll ask Alex Keybl to review.

Comment 12

[Josh Aas]

Comment on attachment 581074 [details] [diff] [review]
Possible fix

Review of attachment 581074 [details] [diff] [review]:
-----------------------------------------------------------------

My apologies for taking so long to get to this. I should have done it earlier or at least said something about it!

::: uriloader/exthandler/mac/nsOSHelperAppService.mm
@@ +264,5 @@
> +// iterate through the entire Launch Services database -- a process which can
> +// take several seconds.)
> +static CFArrayRef GetMIMETypesHandledByApp(FSRef *aAppRef)
> +{
> +  CFURLRef appURL = ::CFURLCreateFromFSRef(kCFAllocatorDefault, aAppRef);

Isn't this leaking in the default non-error case? I only see it released in early returns.

@@ +268,5 @@
> +  CFURLRef appURL = ::CFURLCreateFromFSRef(kCFAllocatorDefault, aAppRef);
> +  if (!appURL) {
> +    return NULL;
> +  }
> +  CFDictionaryRef infoDict = ::CFBundleCopyInfoDictionaryForURL(appURL);

Isn't this also leaking in the default non-error case? I only see it released in early returns.

@@ +270,5 @@
> +    return NULL;
> +  }
> +  CFDictionaryRef infoDict = ::CFBundleCopyInfoDictionaryForURL(appURL);
> +  if (!infoDict) {
> +    CFRelease(appURL);

I think we have a stack-based class somewhere for releasing CF objects. Maybe worth using something like that here? You might have to copy it into the file, but it's small, fewer lines than all the CFRelease calls in this function.

Also, a bunch of these CFRelease calls are missing the :: prefix.

@@ +273,5 @@
> +  if (!infoDict) {
> +    CFRelease(appURL);
> +    return NULL;
> +  }
> +  CFTypeRef type = ::CFDictionaryGetValue(infoDict, CFSTR("CFBundleDocumentTypes"));

It's confusing that you're using the singularly-named variable "type" to refer to an array here, and then later on you use the variable for something entirely different (to reference a single item in this array). Maybe call this "types" and turn the later usage into its own variable called "type".

@@ +297,5 @@
> +      continue;
> +    }
> +    CFDictionaryRef typeDict = static_cast<CFDictionaryRef>(type);
> +
> +    // When this key is present (on OS X 10.5 and later), its contents

This part of the comment seems irrelevant because we don't support anything older than 10.5, but maybe you're just providing extra context?

@@ +310,5 @@
> +    if (!type || (::CFGetTypeID(type) != ::CFArrayGetTypeID())) {
> +      continue;
> +    }
> +    CFArrayRef mimeTypeHolder = static_cast<CFArrayRef>(type);
> +    CFArrayAppendArray(mimeTypes, mimeTypeHolder,

Missing "::" prefix.

@@ +354,5 @@
>    bool extAppIsDefault = false;
>    FSRef typeAppFSRef;
>    FSRef extAppFSRef;
>  
> +  CFStringRef CFMIMEType = NULL;

I don't like stack variables that start with capital letters. I see the temptation here but I think we can find a creative alternative.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (fix problems)

How's this?

> Isn't this leaking in the default non-error case? I only see it
> released in early returns.

Yes, both appURL and infoDict were leaked :-(  I've now fixed this.

> I think we have a stack-based class somewhere for releasing CF
> objects.

I couldn't find one.  And in any case, if we did have one we should
probably use it everywhere in this file -- which really belongs in
another bug.

> Also, a bunch of these CFRelease calls are missing the :: prefix.

Fixed.

>> +  CFTypeRef type = ::CFDictionaryGetValue(infoDict, CFSTR("CFBundleDocumentTypes"));
>
> It's confusing that you're using the singularly-named variable
> "type" to refer to an array here, and then later on you use the
> variable for something entirely different (to reference a single
> item in this array).

I changed it to 'cfObject'.

Methods like CFDictionaryGetValue() and CFArrayGetValueAtIndex() can
return any kind of CoreFoundation object, whose type we won't know
until we call CFType() on it.  I wanted the variable name to reflect
this.  But 'cfObject' is much better for this purpose than 'type'.

>> +    // When this key is present (on OS X 10.5 and later), its contents
>
> This part of the comment seems irrelevant because we don't support
> anything older than 10.5, but maybe you're just providing extra
> context?

I'm providing extra context.

>> +  CFStringRef CFMIMEType = NULL;
>
> I don't like stack variables that start with capital letters. I see
> the temptation here but I think we can find a creative alternative.

I don't either, really.  But I was just copying the style of the
existing code (written by me a while ago).

Normally I don't like to piggyback stylistic changes onto a "real"
fix.  But this time not too many changes were required, and the
likelihood of confusion isn't large.  So I've gone ahead and changed
the names of *all* the stack variables in
nsOSHelperAppService::GetMIMEInfoFromOS() that start with capital
letters.

Comment 14

[Josh Aas]

Comment on attachment 587053 [details] [diff] [review]
Fix rev1 (fix problems)

Review of attachment 587053 [details] [diff] [review]:
-----------------------------------------------------------------

I still think adding the stack-based CFRelease class to the file is the right thing to do but I'm not going to withhold r+ over it. Almost every time this comes up we end up deferring until we have some sort of tree-wide solution, then that never happens so we end up adding more and more fragile code. The class is so small it really doesn't matter in terms of source or compiled code size.

Thanks for fixing this!

::: uriloader/exthandler/mac/nsOSHelperAppService.mm
@@ +264,5 @@
> +// iterate through the entire Launch Services database -- a process which can
> +// take several seconds.)
> +static CFArrayRef GetMIMETypesHandledByApp(FSRef *aAppRef)
> +{
> +  CFURLRef appURL = ::CFURLCreateFromFSRef(kCFAllocatorDefault, aAppRef);

Can't you just release this after the first and only time you use it? Why keep it around only to have to worry about cleaning it up in every error case and at the end of the method?

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 587053 [details] [diff] [review]
Fix rev1 (fix problems)

Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2c762ec80235

I found that both appURL and infoDict could be released immediately after use.  Those changes are in the patch I landed.  Thanks, Josh, for the suggestion.

Comment 16

[Matt Brubeck (:mbrubeck)]

Backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b34af4a3df

because of GetMIMEInfoFromOS crashes in some Mac tests:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8619826&tree=Mozilla-Inbound

(By the way, bugs should stay open after landing on inbound; they'll be resolved when they land on mozilla-central.)

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2c762ec80235
https://hg.mozilla.org/mozilla-central/rev/e5b34af4a3df

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (fix double-free)

(Carry over r+)

(In reply to comment #16)

That crash is due to a dumb mistake on my part -- my rev1 patch can
release cfMimeType twice :-(

Here's a version that fixes the problem.  Just to be safe I'm running
it through the tryservers.  If there's no trouble I'll land it on
mozilla-inbound sometime tomorrow.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 589610 [details] [diff] [review]
Fix rev2 (fix double-free)

Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/23b59d488767

The tryserver builds had no non-spurious failures.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/23b59d488767

Comment 21

[Alex Keybl [:akeybl]]

With FF12's release, instructions like http://www.amazon.com/gp/kindle/mac/download can now be changed to use "Open with" :)