As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 714967 - Export mozilla::services::Get* to external consumers as well
: Export mozilla::services::Get* to external consumers as well
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Justin Wood (:Callek)
: Nathan Froyd [:froydnj]
Depends on:
  Show dependency treegraph
Reported: 2012-01-03 14:46 PST by Justin Wood (:Callek)
Modified: 2012-04-30 07:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (1.31 KB, patch)
2012-01-03 14:46 PST, Justin Wood (:Callek)
benjamin: review-
Details | Diff | Splinter Review
v2 (2.58 KB, patch)
2012-01-04 22:47 PST, Justin Wood (:Callek)
no flags Details | Diff | Splinter Review
v2.1 (2.58 KB, patch)
2012-01-04 22:50 PST, Justin Wood (:Callek)
no flags Details | Diff | Splinter Review
v2.2 (2.58 KB, patch)
2012-01-04 23:05 PST, Justin Wood (:Callek)
benjamin: review+
Details | Diff | Splinter Review

Description User image Justin Wood (:Callek) 2012-01-03 14:46:25 PST
Created attachment 585564 [details] [diff] [review]

The work I am doing in Bug 560772 to make comm-central use mozilla::services will work fine, in internal linkage as per current libxul based code.

I wish to make this API exposed to external code, such that we can link against libxul and use it.

There is a currently supported configuration that Neil (NeilAway) wants to continue to support, that has mailnews built as a separate library linking against libxul rather than *with* libxul. The workaround of doing a wrapper library or additional Macro's for this, I feel is overkill and a bad idea.

I'm not advocating the use of this API for 3rd party consumers, (the do_GetService etc is a better API for those uses) however for our case of primarily internal-to-libxul code, but must-be-available for external linkage, warrants my addition here.

I pushed a patch to try for this at and will follow up with results here soon.

Ben, if you don't think you'll get to the review soon can you name a delegate, and if this needs sr, let me know whom to request from, if not you.
Comment 1 User image Benjamin Smedberg [:bsmedberg] 2012-01-04 13:01:59 PST
The reason we did not do this before, and why I don't want to do it unconditionally now, is that it means that every function which uses mozilla::services within libxul will end up with relocations, and relocations hurt our startup time.

So I don't *think* I want to accept this patch. cc'ing glandium for his thoughts.
Comment 2 User image Nathan Froyd [:froydnj] 2012-01-04 13:09:59 PST
Exporting functions doesn't mean adding relocations, it means that those functions are now called through PLT entries.  That hurts because there is now some extra overhead with each call, and (on x86 Linux, at least), you need to sacrifice %ebx in functions where it wasn't already being used for the GOT pointer.
Comment 3 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-04 14:44:48 PST
So do the standard trick here and have different symbol names for internal and external code.
Comment 4 User image Justin Wood (:Callek) 2012-01-04 22:47:00 PST
Created attachment 585982 [details] [diff] [review]

Alternate approach, suggested by khuey on IRC... I have not yet tested this though... so if there is a simple gotchya feel free to point it out.
Comment 5 User image Justin Wood (:Callek) 2012-01-04 22:50:52 PST
Created attachment 585983 [details] [diff] [review]

v2 approach, with a typo fixed.
Comment 6 User image Justin Wood (:Callek) 2012-01-04 23:05:48 PST
Created attachment 585985 [details] [diff] [review]

One more catch... if you want v1, I need to fix it to have an NS_IMPORT_ in external users as well though.
Comment 7 User image Mike Hommey [:glandium] 2012-01-04 23:48:25 PST
A small note: I don't know about MSVC and OSX's linker, but on Linux, we could use -Wl,-Bsymbolic-functions to make internal calls not go through the PLT, making the #define trick unnecessary (and possibly improving things for all the other exported stuff). We could even use -Bsymbolic, which does more than functions.
Comment 8 User image Benjamin Smedberg [:bsmedberg] 2012-01-06 08:05:55 PST
How would external code *use* this API? It looks like the code would actually end up calling
mozilla::services::_external_GetPrefService, which sounds un-ideal.

Could we instead use a namespace like mozilla::services::external::GetPrefService or even mozilla::external::services::GetPrefService. The latter might be better because you'd end up with something like

using mozilla::external;
And then you'd just be calling services::GetPrefService.

Also, you should be using XPCOM_API(already_AddRefed<TYPE>) instead of NS_IMPORT and NS_EXPORT, which will suppress warnings from the linker about exporting an imported function.
Comment 9 User image Justin Wood (:Callek) 2012-01-06 16:41:32 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Also, you should be using XPCOM_API(already_AddRefed<TYPE>) instead of
> NS_IMPORT and NS_EXPORT, which will suppress warnings from the linker about
> exporting an imported function.

After chatting on IRC you were ok with this, if I switched to XPCOM_API, however I noticed that XPCOM_API uses extern "C" which makes this bad for external use like this.

Can you confirm that this is either ok, or if my patch is ok-as-is given that, (note I do NS_IMPORT_ and NS_EXPORT_ properly based on internal api) or if you know of another suiteable macro.
Comment 10 User image Benjamin Smedberg [:bsmedberg] 2012-01-09 09:02:10 PST
Comment on attachment 585985 [details] [diff] [review]

I misread this patch completely. I wish we had something better than macros. I think this is ok.
Comment 11 User image Serge Gautherie (:sgautherie) 2012-01-10 08:06:29 PST
Comment on attachment 585985 [details] [diff] [review]

Review of attachment 585985 [details] [diff] [review]:

::: xpcom/build/Services.h
@@ +67,5 @@
> +        return _external_Get##NAME();                               \
> +    }
> +
> +#include "ServiceList.h"
> +#undef MOZ_SERVICE

Nit: is it actually needed to duplicate these two lines?
Comment 12 User image Benjamin Smedberg [:bsmedberg] 2012-01-10 08:37:18 PST
It's better to, because then your define/undefine of MOZ_SERVICE match.
Comment 13 User image Justin Wood (:Callek) 2012-01-15 02:32:55 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Comment on attachment 585985 [details] [diff] [review]
> v2.2
Comment 14 User image Justin Wood (:Callek) 2012-01-16 19:29:57 PST
Comment 15 User image 2012-02-19 04:44:17 PST
This exports the symbols from xul itself, rather than any of the other various libraries that we've historically been linking to. Is that a reaonsable thing to do? If so, what's the Makefile magic to link to xul?

Note You need to log in before you can comment on or make changes to this bug.