305949 - Stop exporting nonfrozen XPCOM functions

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Benjamin Smedberg]

Part of the libxul plan to improve performance and reduce embedding headaches is
to stop exporting the nonfrozen symbols. This bug is about the XPCOM symbols.

Comment 1

[Benjamin Smedberg]

Attachment: Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 1

This uses a new macro for the frozen XPCOM API, and keeps NS_COM for internal symbols. It doesn't actually stop exporting the NS_COM symbols from libxul yet, pending tree fixup (see blocking bugs).

Comment 2

[Darin Fisher]

Comment on attachment 201843 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 1

>+#  Can't build internal xptcall tests that use symbols which are not exported.
>+ifndef MOZ_ENABLE_LIBXUL
>+TOOL_DIRS += \
> 	reflect/xptinfo/tests \
> 	reflect/xptcall/tests \
>-	proxy/tests
>+	$(NULL)
>+endif
> endif

We need to figure out how to expose xptcall symbols like
XPTC_InokeByIndex because it is needed to do things like
implement python and java bindings.


>Index: xpcom/string/public/nsStringAPI.h

>+inline char*
>+ToNewUTF8String(const nsAString& aSource)
>+{
>+  nsCString temp;
>+  CopyUTF16toUTF8(aSource, temp);
>+  return NS_CStringCloneData(temp);
>+}

So, we have to decide whether nsStringAPI.h should be an
attempt to implement an API-compatible equivalent to our
internal string API, or if it should be a clean API ;-)
I vote for a cleaner API.  Can we please avoid bringing
in baggage from the internal string API?  Maybe move this
function and others like it into a separate compat header
file?


>+#define PromiseFlatString nsString
>+#define PromiseFlatCString nsCString
>+
>+inline char*
>+ToNewCString(const nsACString& aStr)
>+{
>+  return NS_CStringCloneData(aStr);
>+}

ditto for these...


>+typedef nsCString nsCAutoString;
>+typedef nsString nsAutoString;
>+
>+#define nsXPIDLString nsString
>+#define nsXPIDLCString nsCString

and these...

why not use typedef for nsXPIDLString?


>Index: xpcom/tests/TestMinStringAPI.cpp

>     NS_StringSetData(s, kUnicodeData, PR_UINT32_MAX);
>     len = NS_StringGetData(s, &ptr);
>+    if (len != sizeof(kUnicodeData)/2 - 1)
>       {
>         NS_ERROR("unexpected result");
>         return PR_FALSE;
>       }
>+    if (ptr == nsnull || memcmp(ptr, kUnicodeData, sizeof(kUnicodeData)/2 - 1) != 0)

This looks wrong.  If you are using memcmp, then you are comparing
bytes, so you should pass a length value that expresses the number
of bytes.  I think that sizeof(kUnicodeData) is correct because it
also includes the null terminator, which we can assert exists.


>+    if (ptr == nsnull || memcmp(ptr, kUnicodeData, sizeof(kUnicodeData)/2 - 1) != 0)

ditto.


>-    nsMemory::Free(clone);
>+    NS_Free(clone);

Is nsMemory::Free no longer part of the glue?

Comment 3

[Benjamin Smedberg]

> We need to figure out how to expose xptcall symbols like
> XPTC_InokeByIndex because it is needed to do things like
> implement python and java bindings.

Are we going to freeze them? I don't mind freezing XPTC_InvokeByIndex as-is, but nsXPTCStubBase/nsXPTCVariant should probably be in the glue since there's no good way to freeze it as it depends on the C++ ABI.

> Is nsMemory::Free no longer part of the glue?

It is, but it inlines directly to NS_Free

Comment 4

[Darin Fisher]

> Are we going to freeze them? I don't mind freezing XPTC_InvokeByIndex as-is,
> but nsXPTCStubBase/nsXPTCVariant should probably be in the glue since there's
> no good way to freeze it as it depends on the C++ ABI.

Last time I asked about this, shaver said that these should not be frozen as-is.  I don't know the issues, but I do care about supporting language bindings via extensions, so we need to figure out what we want to do.


> > Is nsMemory::Free no longer part of the glue?
> 
> It is, but it inlines directly to NS_Free

OK, that's what I figured.  You were just changing the code to use NS_Free for aesthetic reasons then ;-)

Comment 5

[Benjamin Smedberg]

Shaver, can we just move all of xptcall into the glue (actually probably its own glue-like library)?

Comment 6

[Benjamin Smedberg]

Attachment: Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2

After discussing with darin, we are going to leave a lot of the CopyUTF8toUTF16 glue in nsStringAPI.h, and mark certain things (nsXPIDLString and nsAutoString) as deprecated. This patch also happens to include bits of bug 315401.

Comment 7

[Darin Fisher]

Comment on attachment 202133 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2

>Index: xpcom/string/public/nsStringAPI.h

>+#define nsXPIDLString                  nsXPIDLString_external
>+#define nsXPIDLCString                 nsXPIDLCString_external

Since these are deprecated, let's move them down to where
nsXPIDLString is defined.


>+inline char*
>+ToNewCString(const nsACString& aStr)
>+{
>+  return NS_CStringCloneData(aStr);
>+}
>+
>+inline PRUnichar*
>+ToNewUnicode(const nsAString& aStr)
>+{
>+  return NS_StringCloneData(aStr);
>+}

I think these should be in the deprecated section since the names of these
functions are terrible.  "ToNewCString"... sounds like "new nsCString" but
it isn't.  "ToNewUnicode"... unicode... what?!?


>+/**
>+ * The following declarations are *deprecated*, and are included here only
>+ * to make porting from existing code that doesn't use the frozen string API
>+ * easier. They may disappear in the future.
>  */
> 
>+class nsXPIDLString : public nsString
>+{
>+public:
>+  operator const PRUnichar*() { return get(); }
>+};
>+
>+class nsXPIDLCString : public nsCString
>+{
>+public:
>+  operator const char*() { return get(); }
>+};

If you are going to support these, what about supporting the NULL return
value from .get() and the cast for nsXPIDLString?  That would seem to be
a very important part of nsXPIDLString that is not being supported here.


> #define EmptyCString() nsCString()
> #define EmptyString() nsString()

EmptyCString/EmptyString are not deprecated.  please move them
out of this section.

Comment 8

[Benjamin Smedberg]

> >+class nsXPIDLString : public nsString

> >+  operator const PRUnichar*() { return get(); }

> If you are going to support these, what about supporting the NULL return
> value from .get() and the cast for nsXPIDLString?  That would seem to be
> a very important part of nsXPIDLString that is not being supported here.

I don't know what you mean, especially WRT to casting. I will try removing these casts and see whether my existing code compiles, until I can figure out what you're talking about ;-)

Comment 9

[Darin Fisher]

> > If you are going to support these, what about supporting the NULL return
> > value from .get() and the cast for nsXPIDLString?  That would seem to be
> > a very important part of nsXPIDLString that is not being supported here.
> 
> I don't know what you mean, especially WRT to casting. I will try removing
> these casts and see whether my existing code compiles, until I can figure out
> what you're talking about ;-)

Take this code for example:

  {
    nsXPIDLString s;
    foo->GetBar(getter_Copies(s));
    if (s) {
      // do stuff with |s|
    }
  }

Notice that this code depends on "nsXPIDLString::operator const PRUnichar*()" evaluating to null when GetBar returns a null out param.  nsString::get() never returns null, so nsString is not a substitute for nsXPIDLString in the above sample code.  This is a very common use-case for nsXPIDLString, so I suspect that you must have encountered code that is depending on this.

Comment 10

[Benjamin Smedberg]

Attachment: Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in]

OK, I didn't need the xpidlstring stuff... Since this doesn't flip the NS_COM switch yet, I'd to land this as-is and then look at how to deal with the xptcall stuff.

Comment 11

[Darin Fisher]

Comment on attachment 202253 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in]

r=darin with the typedefs for nsXPIDLC?String removed from nsStringAPI.h -- it's just really worrisome to have those there if they do not have the "null" property that I mentioned previously.  they are likely to be misused.

Comment 12

[Benjamin Smedberg]

Comment on attachment 202253 [details] [diff] [review]
Fix up macros and add frozen string utilities, but don't flip the switch yet, rev. 2.1 [checked in]

Attachment 202253 [details] [diff] checked in with xpidlc?string removed and CopyUTF16toASCII renamed with a Lossy to match the nonfrozen version.

Comment 13

[Benjamin Smedberg]

Attachment: Stop exporting GFX symbols, rev. 1 [checked in]

Comment 14

[Benjamin Smedberg]

Attachment: Various XPCOM macros, rev. 1 [checked in]

I'm thinking that we never want to override IMETHOD_VISIBILITY to NS_VISIBILITY_DEFAULT, always preferring to define it to empty and use the class-scope visibility declaration.

Comment 15

[Darin Fisher]

> I'm thinking that we never want to override IMETHOD_VISIBILITY to
> NS_VISIBILITY_DEFAULT, always preferring to define it to empty and use the
> class-scope visibility declaration.

And that works properly on Linux?  NS_COM is empty on Linux, no?

Comment 16

[Benjamin Smedberg]

No, http://lxr.mozilla.org/mozilla/source/xpcom/base/nscore.h#282

Comment 17

[Darin Fisher]

Comment on attachment 218722 [details] [diff] [review]
Various XPCOM macros, rev. 1 [checked in]

ok, but i think bryner should review this.  i thought that older compilers lacked support for specifying visibility at the class level.

Comment 18

[Benjamin Smedberg]

They do, but on those compilers we don't use pragmas nor -fvisibility=hidden, so the default visibility is fine.

Comment 19

[Brian Ryner (not reading)]

Comment on attachment 218722 [details] [diff] [review]
Various XPCOM macros, rev. 1 [checked in]

I wonder if we should change the way the visibility macros work so that it's not necessary to duplicate this somewhat non-intuitive #defining.  In any case, sr=me.

Comment 20

[Benjamin Smedberg]

Attachment: No need to export widgetshared or mozreg symbols, rev. 1 [checked in]

The widget classes are not NS_EXPORT or called across dll boundaries, no need to override the default visibility. The NSReg symbols are all statically linked, no need for dllexport mojo. The OJI symbol just needed visibility love.

Comment 21

[Benjamin Smedberg]

This is done well enough to call it fixed.