Closed Bug 442401 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: hwaara, Assigned: hwaara)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Here's approach number one, for reference.
Attached patch Approach #2: Use Cocoa (obsolete) — Splinter Review
Approach #2 which uses Cocoa. This is slick. Requesting review on this one, and would like to get it into mozilla-central.
Attachment #327201 - Flags: review?(joshmoz)
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?
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 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.
Attachment #327200 - Attachment is obsolete: true
Attachment #327201 - Flags: superreview+
Attachment #327201 - Flags: review?(joshmoz)
Attachment #327201 - Flags: review+
Attachment #327201 - Flags: superreview+ → superreview?(benjamin)
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.
Assignee: nobody → hwaara
Status: NEW → ASSIGNED
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.
Attachment #327201 - Flags: superreview?(benjamin) → superreview?(joshmoz)
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.
Attachment #327201 - Flags: superreview?(joshmoz)
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.
This one also follows links.
Attachment #327201 - Attachment is obsolete: true
Attachment #333201 - Flags: review?(joshmoz)
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?
Attached patch Patch v1.2 (obsolete) — Splinter Review
> 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.
Attachment #333201 - Attachment is obsolete: true
Attachment #334123 - Flags: review?(joshmoz)
Attachment #333201 - Flags: review?(joshmoz)
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.
Attached patch Patch v1.5 (obsolete) — Splinter Review
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.
Attachment #334123 - Attachment is obsolete: true
Attachment #335299 - Flags: review?(joshmoz)
Attachment #334123 - Flags: review?(joshmoz)
Attachment #335299 - Flags: superreview?(roc)
Attachment #335299 - Flags: review?(joshmoz)
Attachment #335299 - Flags: review+
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.
Attachment #335299 - Flags: superreview?(roc)
(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?).
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.
Attached patch Patch v2Splinter Review
I managed to cut the size of the binary by around ~100K by removing all resources possible in it (menus etc). Is that alright?
Attachment #335299 - Attachment is obsolete: true
Attachment #335496 - Flags: superreview?(roc)
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.
Attachment #335496 - Flags: superreview?(roc) → superreview+
Pushed eb9386fca111 and f400cbec8311 for a fixup of the test.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 452206
You need to log in before you can comment on or make changes to this bug.