646748 - "Unknown error" occurs when trying to open a file with Across Lite from Firefox

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Adam Rosenfield]

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).

Comment 1

[Adam Rosenfield]

Attachment: 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 2

[Jeff Walden [:Waldo]]

Comment on attachment 543290 [details] [diff] [review]
Proposed fix

Josh moved to working on networking code fairly recently, redirecting...

Comment 3

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 4

[Adam Rosenfield]

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.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Oh, and the fix for bug 663899 *is* also intended to be cross-platform.

Comment 7

[Adam Rosenfield]

In that case, I need to fix my patch, which was based on an incorrect assumption.

Comment 8

[Adam Rosenfield]

Attachment: 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.

Comment 9

[Adam Rosenfield]

Attachment: Proposed fix v2.1

Fixed typo in comment

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Attachment: 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.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Oh, and since I'm able to reproduce this bug, I'll change the status to NEW.

Comment 12

[Adam Rosenfield]

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 13

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f

Comment 15

[Adam Rosenfield]

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.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

I'll wait for you to take care of it tonight.

If you haven't by tomorrow, I'll do it myself.

Comment 17

[Adam Rosenfield]

Attachment: Fix v3.0

Here goes; let me know if I didn't get the flags right.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

It all looks fine to me.

Thanks for your patch!

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/6632abc4dd0a