442401 - nsILocalFile::IsPackage does not consider a keynote bundle (.key) to be a package

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Håkan Waara]

The implementation of IsPackage is really hacky. It does not consider a keynote package to be a package, for example. It works on apps because it's cheating and checking for the ".app" suffix. 

The reason is that we're using a bad API that, from what I understand, only checks if the bundle bit of a file - which may or may not be set. This is a widely known problem (see for example http://coding.derkeiler.com/Archive/Python/comp.lang.python/2004-11/2075.html).

There are two ways to do this correctly:

1) Use Carbon and Launch Services. All Apple sample code that wants to find out if a file/folder is a package, uses LSCopyItemInfoForRef() and checks if the kLSItemInfoIsPackage bit is set. (Example: http://developer.apple.com/technotes/tn/tn2017.html#Section6)

2) Simply use NSWorkspace and isFilePackageAtPath:(NSString *)path

I've tried both approaches and both solve the problem. #2 is much neater and simpler, and since we're moving in the direction of Cocoafying these implementations anyway, I prefer it.

Comment 1

[Håkan Waara]

Attachment: Approach #1: Use Carbon (ugh) and Launch Services

Here's approach number one, for reference.

Comment 2

[Håkan Waara]

Attachment: Approach #2: Use Cocoa

Approach #2 which uses Cocoa. This is slick. Requesting review on this one, and would like to get it into mozilla-central.

Comment 3

[Håkan Waara]

I'm gonna be away for a while, but I've been thinking about also providing a unit test which would include a (small) .key file that it could check for package-ness.

Does that sound reasonable?

Comment 4

[Håkan Waara]

roc or bsmedberg, is there someone else that can review the patch (Approach #2) above? 

I hear that josh is really busy, and the patch has been sitting for one and a half month...

nsILocalFile::IsPackage is basically broken, and the fix is really simple, because we defer all work to Cocoa.

Comment 5

[Håkan Waara]

Comment on attachment 327200 [details] [diff] [review]
Approach #1: Use Carbon (ugh) and Launch Services

Because I strongly doubt anyone wants this Carbon approach, I'm gonna mark it obsolete. 

It's mainly attached for comparison's sake.

Comment 6

[Håkan Waara]

Attachment: Testcase for mac bundles

Here's also a xpcshell testcase for this bug. Feel free to review.

I've added a new directory "data" in xpcom/tests/unit with two bundles to be used as resources for this test: 

* "dummy.app" which is a minimal mac application
* "presentation.key" which is a minimal Keynote presentation

The keynote presentation fails to be recognized as a bundle without my fix.

Comment 7

[Håkan Waara]

Comment on attachment 327201 [details] [diff] [review]
Approach #2: Use Cocoa

Josh, please have a look. I'm using NSWorkspace's isFilePackageAtPath: which will correctly determine bundleness for us.

I already attached xpcshell a testcase that with this patch will pass.

Comment 8

[Josh Aas]

Comment on attachment 327201 [details] [diff] [review]
Approach #2: Use Cocoa

The old code used "GetFSRefInternal" to get the path in the form of an FSRef. Obviously we don't want an FSRef but part of what that function did is resolve filesystem links like this:

CFURLRef whichURLRef = mFollowLinks ? mTargetRef : mBaseRef;

Your new code uses mBaseRef every time. Please fix this or explain why you think what you're doing is right.

Comment 9

[Håkan Waara]

Good point. So this is about the nsILocalFile attribute "followLinks" which determines if some nsILocalFile[Mac] APIs should work on the file at hand, or - if it is an alias - on the target of that alias.

I can make it work on the target, like the old version.

Comment 10

[Håkan Waara]

Attachment: Patch v1.1: also support mac aliases

This one also follows links.

Comment 11

[Josh Aas]

Comment on attachment 333201 [details] [diff] [review]
Patch v1.1: also support mac aliases

+    ::CFURLCopyFileSystemPath((mFollowLinks ? mTargetRef : mBaseRef), kCFURLPOSIXPathStyle);

Are you sure you don't need to null check mTargetRef or mBaseRef?

Comment 12

[Håkan Waara]

Attachment: Patch v1.2

> Are you sure you don't need to null check mTargetRef or mBaseRef?

Good call.

This patch also checks both mBaseRef and mTargetRef for null-ness before using any of them.

Comment 13

[Josh Aas]

Comment on attachment 334123 [details] [diff] [review]
Patch v1.2

+  CFURLRef whichURLRef = (mFollowLinks && mTargetRef) ? mTargetRef : mBaseRef;

I think you should return an error if mFollowLinks is true but there is no mTargetRef rather potentially targeting the wrong file (mBaseRef) in that error condition.

Maybe you should just use the existing nsLocalFile::GetCFURL. If you do that be sure to release the result as GetCFURL retains.

Comment 14

[Håkan Waara]

Attachment: Patch v1.5

This patch uses GetCFURL instead, which further simplifies the code. I could also convert the ugly CF API call to get the URL's path as a string to a simple objc message because NSURL and CFURLRef are toll-free bridged.

Comment 15

[Josh Aas]

Comment on attachment 335299 [details] [diff] [review]
Patch v1.5

The r+ applies to the part of the patch you meant to include - you accidentally included a whole bunch of other stuff. Please post a new patch without the extra stuff and then request sr.

Comment 16

[Håkan Waara]

(In reply to comment #15)
> (From update of attachment 335299 [details] [diff] [review])
> The r+ applies to the part of the patch you meant to include - you accidentally
> included a whole bunch of other stuff. Please post a new patch without the
> extra stuff and then request sr.

The extra stuff are testcase resources:

* one sample application ("dummy.app")
* one sample keynote file ("presentation.key")

these are used in the testcase as bundles that are checked. They can also be reused in the future in other mac xpcom testcases if needed (for example, a testcase for opening apps? or testing whether an nsIFile (dummy.app) is executable?).

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is dummy.app/Contents/Resources/English.lproj/MainMenu.nib/designable.nib really as small as it can be? If there's an easy way to shrink it, let's do that.

Comment 18

[Håkan Waara]

Attachment: Patch v2

I managed to cut the size of the binary by around ~100K by removing all resources possible in it (menus etc). Is that alright?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 335496 [details] [diff] [review]
Patch v2

That'll do. If there was a way to shrink the Keynote file, I'd do that too, but this is OK as is.

Comment 20

[Håkan Waara]

Pushed eb9386fca111 and f400cbec8311 for a fixup of the test.