User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0 Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.2a1pre) Gecko/20110330 Firefox/4.2a1pre When attempting to open a .puz file in Firefox using Across Lite, I get the following error dialog: ---------- snip ---------- <b>Download Error</b> filename.puz could not be opened, because an unknown error occurred. Try saving to disk first and then opening the file. ---------- snip ---------- Saving to disk and opening the file is successful, but that's rather annoying. Reproducible: Always Steps to Reproduce: 1. Download and install Across Lite software for Mac from <http://www.nytimes.com/downloads/acl2mac.dmg> 2. Click on a link to a .puz file, such as <http://icrossword.com/publish/server/puzzle/index.php/mgwcc_147.puz?id=BDDB_mgwcc_147.puz> 3. In the "What should Firefox do with this file?" dialog, click the "Choose..." button, navigate to the Across Lite v2.0 executable, and click Open 4. Click OK Actual Results: The "unknown error" dialog is shown as described in the Details. The .puz file fails to download or be opened in Across Lite. Expected Results: The .puz file is downloaded and opened in Across Lite. This bug appeared after upgrading from Firefox 3.6 to 4.0, with no changes to Across Lite. This bug happens both when choosing Across Lite from the "What should Firefox do with this file?" dialog and when Across Lite is set as the default application for opening .puz files (which I'd had with FF 3.6). This bug seems to be specific to Across Lite: this bug does not appear when trying to open .puz files with other applications (e.g. TextEdit).
Created attachment 543290 [details] [diff] [review] Proposed fix Oh what the hell, I fixed it myself. This bug was introduced by changeset 9c005e5d9719 in the fix for bug 571193. Related to bug 663899 (though I don't have access to that bug). This bug was occurring because Across Lite is a Mac Classic application with permissions of -rw-r--r--, so it was considered to be non-executable, even though it is actually executable according to Launch Services.
Comment on attachment 543290 [details] [diff] [review] Proposed fix Josh moved to working on networking code fairly recently, redirecting...
Comment on attachment 543290 [details] [diff] [review] Proposed fix I'm busy with more urgent bugs, and it'll take me a while to fully understand the implications of your patch (particularly of getting rid of the special cases for *.jar and *.air files). So it'll probably be a few weeks before I can get to this. If it turns out I can't get to it within that time, I'll try to find another reviewer.
Ok. I don't have access to bug 663899, so can't verify if there have been any regressions. Was that bug for just Mac OS X, or was it occurring for all *nix flavors? My patch was made on the assumption that that bug was only for OS X; if that's not the case, then the special case for .jar and .air files should be kept.
Bug 663899 is a security bug (which is why access is restricted). So it's important to get these changes right :-) Also (as I myself failed to notice earlier) the code you've changed *is* cross-platform (meant to be used with any Unix-style file system). So at the very least any Mac-specific code would have to be ifdefed, and there's some question whether we should put any Mac-specific code there at all. I need to wait until I'm not distracted by other things, and can devote my full attention to these problems.
Oh, and the fix for bug 663899 *is* also intended to be cross-platform.
In that case, I need to fix my patch, which was based on an incorrect assumption.
Created attachment 544394 [details] [diff] [review] Proposed fix v2 This fixes this bug without affecting the behavior with regards to bug 663899; this patch affects only the Mac OS X build.
Created attachment 544395 [details] [diff] [review] Proposed fix v2.1 Fixed typo in comment
Created attachment 547483 [details] [diff] [review] v 2.1 fix with narrowed scope I've finally gotten time to work on this. Your patch is basically fine, but I'd prefer something like this, which makes a smaller change to existing code. For example, it still checks the executable bit if Launch Services doesn't identify something as executable.
Oh, and since I'm able to reproduce this bug, I'll change the status to NEW.
That seems sensible; I was copying the behavior pre-9c005e5d9719 (lines 1017-1040 of http://hg.mozilla.org/mozilla-central/diff/9c005e5d9719/xpcom/io/nsLocalFileOSX.mm), which just returned PR_FALSE if an error occurred. I'm not sure what can cause GetCFURL or LSCopyItemInfoForRef to fail.
Comment on attachment 547483 [details] [diff] [review] v 2.1 fix with narrowed scope > I'm not sure what can cause GetCFURL or LSCopyItemInfoForRef to > fail. I don't know, either. But I think it makes sense to treat a failure in GetCFURL() more seriously than one in LSCopyItemInfoForRef(), as both our patches do. So if you're happy with my suggestions, the next steps are: 1) Create a copy of "my" patch (the one from comment #10) that has your name as the developer, but which is easy for someone else to land: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_chec\ k-in_for_me.3f 2) Add "checkin-needed" to the Keywords field. This will put the patch in the queue for being landed on the mozilla-inbound tree, which is periodically (about once a day) merged to the mozilla-central tree, which is in effect the "trunk". Or if you'd prefer I can do this for you.
If it's easy for you to do, then go ahead. Otherwise I'll take care of it when I get home to my Mac later tonight.
I'll wait for you to take care of it tonight. If you haven't by tomorrow, I'll do it myself.
Created attachment 547611 [details] [diff] [review] Fix v3.0 Here goes; let me know if I didn't get the flags right.
It all looks fine to me. Thanks for your patch!