636841 - Mac App Store rejects embedded XULRunner for private APIs

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: Mac, defect)

Description

[Chad Austin]

IMVU's installable client is based on embedded XULRunner, but the Mac App Store rejected us because XULRunner uses two private APIs:

http://mxr.mozilla.org/mozilla1.9.1/search?string=_NSGetCarbonMenu
http://mxr.mozilla.org/mozilla1.9.1/search?string=_LSCopyDefaultSchemeHandlerURL

We're currently running 1.9.1, blocked on upgrading to 1.9.2 due to https://bugzilla.mozilla.org/show_bug.cgi?id=533245.  Either way, it looks like 1.9.2 also uses these APIs.

What would it take to remove these API calls from XULRunner so the Mac App Store would accept us?

Comment 1

[passfree]

I am interested to know if anyone has any ideas about this?

Comment 2

[passfree]

Attachment: patch with fix

I've made a patch which fixes this issue. I don't know why the original author of the file suggests that LSGetApplicationForURL will fail. In my tests it works all them time.

Can someone review the code and merge with central if approved? If the worry is that this fix wont be appropriate, then can we add build option for mac app store compatibility? i.e.

ac_add_options --mac-app-store

or something like this to turn on the feature.

Thanks

Comment 3

[passfree]

Attachment: sample test application

This sample xul application tests the patch. As you can see it works well for all default url handlers.

Comment 4

[Josh Matthews [:jdm]]

You should probably get rid of the declaration of _LSCopyDefaultSchemeHandlerURL as well.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 576475 [details]
sample test application

> You should probably get rid of the declaration of
> _LSCopyDefaultSchemeHandlerURL as well.

Please attach a new version of your patch that does this.  I'll r+ it
and land it on mozilla-inbound (from which it will quickly find its
way onto mozilla-central).

> the Mac App Store rejected us because XULRunner uses two private
> APIs:
>
> http://mxr.mozilla.org/mozilla1.9.1/search?string=_NSGetCarbonMenu
> http://mxr.mozilla.org/mozilla1.9.1/search?string=_LSCopyDefaultSchemeHandlerURL

The Mozilla tree uses many more private APIs than these two.  It will
be impossible to remove them all.  Apple's own apps use private APIs
(I'll have more to say about this in a later comment).  So must any
app above a certain level of complexity.

It's hypocritical for Apple to reject apps from the App Store that use
private APIs, and pointless for Mozilla to try to meet this demand in
full.

Nonetheless, you seem to have found us using one private API
(_LSCopyDefaultSchemeHandlerURL()) that we no longer need to use.  The
public API with which you replace it (LSGetApplicationForURL()) is
used elsewhere in the tree (in nsMacShellService.cpp), apparently
without any problems.  And in my (brief) tests of your patch, I didn't
see any problems, either.

_NSGetCarbonMenu is no longer used as of FF 4.0.

The 1.9.1 branch is no longer maintained.  There's no chance that
we'll backport this bug's patch to the 1.9.2 branch (the FF 3.6.X
branch).

Comment 6

[Steven Michaud [:smichaud] (Retired)]

I tracked our first use of _LSCopyDefaultSchemeHandler() back to 2005,
in the patch for bug 264648 (see particularly bug 264648 comment #5
and bug 264648 comment #7).  Neither the bug nor the code comments say
much about LSGetApplicationForURL()'s failures.

I'm not a bit surprised that there *were* failures -- Launch Services
is complex, and I've seen quite a few myself (with different methods).
But whatever they were, they seem to have disappeared a while ago.

Nonetheless we'll need to keep our eyes open for problems that may be
caused by no longer using _LSCopyDefaultSchemeHandler().

Comment 7

[Steven Michaud [:smichaud] (Retired)]

As I mentioned above in comment #5, Apple's own apps use private APIs.

I've confirmed this with Safari.  But I'm quite sure other Apple apps
also do so.  It's quite easy to find out.  Here's how to do it:

Find an application binary (usually in the app bundle's
Contents/MacOS/ directory) and run 'ns -pam' on it.  This will print
all of the binary's symbols to stdout.  Among them will be many
symbols imported from other binaries, some of which will be system
libraries or frameworks.  All of their names will be prefixed with an
extra underscore character.  Here are two examples, from the output
for Safari:

(undefined [lazy bound]) external _abort (from libSystem)
(undefined [lazy bound]) external __LSCopyDefaultSchemeHandlerURL (from CoreServices)

'abort' is (of course) a standard POSIX call, and well documented.
'_LSCopyDefaultSchemeHandlerURL' is, as we already know, not
documented :-)

Note that Safari's use of private APIs is doubly private: 

The Safari binary itself has almost no public symbols.  But it links
to the Safari framework in /System/Library/PrivateFrameworks.  It's
from there that I got the two symbols listed above.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stack showing Safari directly calling _LSCopyDefaultSchemeHandlerURL

(Following up comment #7)

To further confirm what I said about Safari using private APIs, here's
a stack of _LSCopyDefaultSchemeHandlerURL being	called directly from
code in the Safari framework (specifically from the
dictionaryForScheme: method of the DefaultWebAppPopUpController class).

Note that _LSCopyDefaultSchemeHandlerURL is not being called from some
other, documented Launch Services API.

To get this stack, I ran Safari in gdb,	set a breakpoint on
_LSCopyDefaultSchemeHandlerURL, and chose Safari : Preferences.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> 'ns -pam'

Oops, sorry.  This should have been 'nm -pam'.

Comment 10

[Josh Matthews [:jdm]]

Steven, I didn't get a sense of whether your comments indicated you were in favour of this patch or not. Could you make a call one way or the other instead of this limbo?

Comment 11

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #10)

See comment #5, particularly the following:

> Please attach a new version of your patch that does this.  I'll r+ it
> and land it on mozilla-inbound (from which it will quickly find its
> way onto mozilla-central).

As I've explained above, it's a fool's errand to stop using private APIs altogther.  Apple knows this as well as I do, judging from it's own (i.e. Safari's) use of private APIs.

But, as I already stated quite clearly in comment #5, I'm happy to stop using particular private APIs if there are reasonable non-private alternatives.

I've been waiting all this time for someone to follow my recommendation, or to give some reason for not following it.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> But, as I already stated quite clearly in comment #5, I'm happy to
> stop using particular private APIs if there are reasonable
> non-private alternatives.

Even if (as in this case) Safari continues to use the very private API
that we'd no longer be using.

Comment 13

[passfree]

I think that we should aim to make developers' lives easier. If apple is not approving private api than perhaps we should limit their use. There are two reasons not to use private apis:
     * Imagine the benefit you will get if more xul-based applications appear on the appstore. I am sure that this will increase the number of people messing with the mozilla platform and contributing bug fixes, etc. I am not one of the most avid contributors since I just work for myself and I don't have a lot of time on my hands but nevertheless I try to help out where I can spare resources. This is all due to the fact that I can use xulrunner for my job. IMHO the more people there are out there like me the better because we do resilience testing of mozilla's code under many different constraints and when we find a bug we either fix it, report it or lobby to get it fixed.
     * Private apis are subject to change. Tomorrow Apple may decide to release an update which removes some of these APIs or limit their use. That will require mozilla to release an update for all mac os x users which will turn into disaster because the update will fail due to ff wont be able to launch at first place. Apple is certainly moving towards the iOS way with lion so this is more than anticipated at the moment.

Now, if there are situations where it is simply not possible to use anything else but private APIs then fair enough. As far as I know my patched version (patch above) of xulrunner passes the appstore approval process. Perhaps I am not hitting the rest of the private APIs and therefore are not detected... it may as well be a security bug with appstore (I work in security, so I will not discuss it here) who knows.

What I want to get through is that at least the patch which I have provided (with minor corrections, we need to remove the external definition) works and eliminates one instance of private api usage which we can go without.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #13)

You've given the standard argument against using private APIs, and
it's entirely beside the point.  As I've already said above, there are
two main reasons for this:

1) Any app above a certain level of complexity needs to use private
   APIs.  This includes	Firefox and Safari.  I know this to be true
   from personal experience on both OS X and Windows.

2) Apple's own apps and frameworks use private APIs on a major scale.
   So it's hypocritical of Apple to attempt to make other vendors'
   apps not use private APIs.

Now if Apple started a program to solicit requests from other vendors
to make public certain private APIs, and was willing to approve all
reasonable requests, the situation would be entirely different.  But
this certainly hasn't been how Apple has behaved up til now.

I'm not suggesting that apps should use just any private API.  There
should be no decent public alternative.  And it should be APIs that
the OS vendors own apps use, and which ideally haven't changed for the
last several major versions of the OS.

I know from my own experience that private APIs used by Apple's own
apps *don't* change in minor updates -- for the	very good reason that
this would inconvenience Apple's own developers.  They sometimes do
change in major updates, and one needs to watch out for that.  But I
don't know of a single case where this has happened with a private API
used by Firefox.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

(In further reply to comment #13)

As I've already said, more than once, I'm happy to r+ your patch if
you revise it according to my suggestion (actually Josh Matthews'
suggestion) from comment #5.  Then I can land it in the tree, and it
will appear in trunk nightlies in the next few days.

I'm also willing to listen to suggestions that we stop using other
private APIs.  But all of these private APIs were originally used for
a reason, and that reason may still be valid.

I don't think the current situation justifies a major effort (on my
part on anyone else's) to identify *all* the private APIs we use, and
search for replacements for them.

Yes, you may think that you've stepped in a hornet's nest.  I'm
sorry for that.  But it's there for a good reason :-)

I've no	idea why Apple's App Store approval process identified only
those private APIs you mentioned in comment #0.  There could be any
number of reasons.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Passfree's patch with Josh Matthews' suggested change

I got tired of waiting.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 596763 [details] [diff] [review]
Passfree's patch with Josh Matthews' suggested change

Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/27099c5bfca1

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/27099c5bfca1

Comment 19

[passfree]

Thanks!

Comment 20

[abarnert]

The patch as committed leaves behind the comments explaining why _LSCopyDefaultSchemeHandlerURL is being used, which is misleading given that it's not being used anymore…

Comment 21

[Steven Michaud [:smichaud] (Retired)]

In reply to comment #20)

Where?

Comment 22

[abarnert]

Sorry, never mind, it looks like that comment is referring to NSURLFileTypeMappingsInternal, not to _LSCopyDefaultSchemeHandlerURL.

Comment 23

[abarnert]

Any change this change could be backported to the 10.0 branch, so any future ESR releases will have it?