Closed
Bug 675356
Opened 13 years ago
Closed 12 years ago
Dmg files sometimes not handled correctly
Categories
(Toolkit :: Downloads API, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: jrmuizel, Assigned: smichaud)
References
Details
(Whiteboard: [inbound])
Attachments
(2 files, 2 obsolete files)
450 bytes,
patch
|
Details | Diff | Splinter Review | |
13.36 KB,
patch
|
smichaud
:
review+
|
Details | Diff | Splinter Review |
Instead I get the following message: The application you chose ("") could not be found. Check the file name or choose another application.
FWIW, I've always gotten this behavior and thought it was just my machine until a clean OS X install today.
Reporter | ||
Updated•13 years ago
|
Keywords: regressionwindow-wanted
Comment 2•13 years ago
|
||
Assignee: nobody → akeybl
Status: NEW → ASSIGNED
Comment 3•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee: akeybl → nobody
Status: ASSIGNED → NEW
Comment 5•13 years ago
|
||
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?
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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".
Assignee | ||
Comment 8•13 years ago
|
||
Thanks, Alex. This bug now makes some sense :-)
Assignee | ||
Comment 9•13 years ago
|
||
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).
Assignee | ||
Updated•13 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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.
Attachment #581074 -
Flags: review?(joshmoz)
Assignee | ||
Updated•12 years ago
|
Summary: Opening dmg files directly from the download prompt no longer works → Dmg files sometimes not handled correctly
Comment 12•12 years ago
|
||
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.
Attachment #581074 -
Flags: review?(joshmoz) → review-
Assignee | ||
Comment 13•12 years ago
|
||
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.
Attachment #581074 -
Attachment is obsolete: true
Attachment #587053 -
Flags: review?(joshmoz)
Comment 14•12 years ago
|
||
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?
Attachment #587053 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 15•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 16•12 years ago
|
||
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.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2c762ec80235 https://hg.mozilla.org/mozilla-central/rev/e5b34af4a3df
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Attachment #587053 -
Attachment is obsolete: true
Attachment #589610 -
Flags: review+
Assignee | ||
Comment 19•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [inbound]
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/23b59d488767
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Comment 21•12 years ago
|
||
With FF12's release, instructions like http://www.amazon.com/gp/kindle/mac/download can now be changed to use "Open with" :)
You need to log in
before you can comment on or make changes to this bug.
Description
•