763848 - Make it easier to link to libxul with js-ctypes

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

In some cases, the simplest way of exporting new C functions for use with js-ctypes is to add these functions to libxul and link to libxul with js-ctypes. Unfortunately, for some reason, linking to libxul with js-ctypes is non-trivial/not possible on some platforms – right now, I am thinking of MacOS X from a Chrome Worker.

I suggest making the process simple for end-users. I believe that we could tweak the current API so that calling |ctypes.open()| opens libxul.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I suspect that the easiest would be to publish the location of libxul in OS.Constants.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

Same one, with a trivial typo fixed and a few additional comments.
Gaston, if you have some time, could you check whether this works on *BSD?

Comment 5

[Landry Breuil (:gaston)]

You're going to hate me.. but your diff fails to compile with :



/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:363: error: invalid conversion from 'const char*' to 'PRUnichar'
/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:363: error:   initializing argument 1 of 'void nsAString_internal::Append(
PRUnichar)'
/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:365: error: invalid conversion from 'const char*' to 'PRUnichar'
/home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:365: error:   initializing argument 1 of 'void nsAString_internal::Append(
PRUnichar)'

Trying with DLL_PREFIX/SUFFIX wrapped without NS_LITERAL_STRING...

Comment 6

[Landry Breuil (:gaston)]

err, within, of course.


+    xulName.Append(NS_LITERAL_STRING(DLL_PREFIX));
+    xulName.Append(NS_LITERAL_STRING("xul"));
+    xulName.Append(NS_LITERAL_STRING(DLL_SUFFIX));

builds here, will do the actual testing once it finishes

Comment 7

[Landry Breuil (:gaston)]

1 INFO TEST-START | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul
2 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Starting test
3 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Chrome worker created
4 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Test in progress
5 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_xul: opened libxul successfully
6 INFO TEST-END | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | finished in 282ms
7 INFO TEST-START | Shutdown
8 INFO Passed: 4

So with NS_LITERAL_STRING(), all good for me

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

Thanks for the BSD test, gaston.
You are, of course, right about the NS_LITERAL_STRING. I had forgotten to update the patch after testing it under Windows.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 636063 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Review of attachment 636063 [details] [diff] [review]:
-----------------------------------------------------------------

NS_GetSpecialDirectory is not thread-safe.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 636063 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Review of attachment 636063 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/OSFileConstants.cpp
@@ +412,5 @@
> +    // On other platforms, libxul is a library "xul" with regular
> +    // library prefix/suffix
> +    xulName.Append(NS_LITERAL_STRING(DLL_PREFIX));
> +    xulName.Append(NS_LITERAL_STRING("xul"));
> +    xulName.Append(NS_LITERAL_STRING(DLL_SUFFIX));

You probably want xulName.Append(NS_LITERAL_STRING(DLL_PREFIX "xul" DLL_SUFFIX));

Comment 11

[David Teller [:Yoric] - still alive but not very active]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> Comment on attachment 636063 [details] [diff] [review]
> Exposing path to libxul in OS.Constants
> 
> Review of attachment 636063 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> NS_GetSpecialDirectory is not thread-safe.

Great :/

Ok, so plan B is the following:
- extract path to libxpcom during initialization on the main thread;
- re-publish this information as a global string;
- use that global string during initialization of OS.Constants.Sys;
- cry a river for all the other constants I will want to expose to OS.File.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

Attaching an updated version. As suggested by khuey, it extracts the path name from the main thread, during initialization of the ChromeWorker service, and uses this information once chrome workers are effectively created.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 637120 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Review of attachment 637120 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not going to r- it this time, but I would like to see it again before giving r+.

::: dom/system/OSFileConstants.cpp
@@ +48,5 @@
> +
> +/**
> + * The name of the directory holding all the libraries (libxpcom, libnss, etc.)
> + */
> +nsString gLibDirectory;

Place these global variables in an anonymous namespace please.

@@ +65,5 @@
> +  gInitialized = true;
> +
> +  // Initialize gLibDirectory
> +   nsCOMPtr<nsIFile> xpcomLib;
> +   nsresult rv = NS_GetSpecialDirectory("XpcomLib", getter_AddRefs(xpcomLib));

Please fix your indentation in this function.

@@ +67,5 @@
> +  // Initialize gLibDirectory
> +   nsCOMPtr<nsIFile> xpcomLib;
> +   nsresult rv = NS_GetSpecialDirectory("XpcomLib", getter_AddRefs(xpcomLib));
> +   if (!NS_SUCCEEDED(rv) || !xpcomLib) {
> +     return false;

Do not return false from a function that has an nsresult retval.

@@ +73,5 @@
> +
> +   nsCOMPtr<nsIFile> libDir;
> +   rv = xpcomLib->GetParent(getter_AddRefs(libDir));
> +   if (!NS_SUCCEEDED(rv)) {
> +     return false;

Do not return false from a function that has an nsresult retval.

@@ +114,5 @@
>  #define S_IRUSR 0400
>  #define S_IRWXU 0700
>  #endif // !defined(S_IRGRP)
>  
> +

Unnecessary newline addition.

@@ +428,5 @@
> +#endif // defined(XP_MACOSX)
> +
> +    JSString* strPathToLibXUL = JS_NewUCStringCopyZ(cx, xulPath.get());
> +    jsval valXul = STRING_TO_JSVAL(strPathToLibXUL);
> +    JS_SetProperty(cx, objSys, "libxulpath", &valXul);

Why don't you care about the return value here.

::: dom/workers/RuntimeService.cpp
@@ +931,5 @@
>                                       mSystemCharset);
>    }
>  
> +  rv = InitOSFileConstants();
> +  if(!NS_SUCCEEDED(rv)) {

Nit: if (

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

Thanks for the quick review. Applied your changes.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 637143 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Review of attachment 637143 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/OSFileConstants.cpp
@@ +46,5 @@
> +
> +/**
> + * The name of the directory holding all the libraries (libxpcom, libnss, etc.)
> + */
> +nsString gLibDirectory;

This isn't an anonymous namespace.  Please put them in a "namespace { };" block so that if anyone does "extern gInitialized" in the future it won't latch on to this.

::: dom/workers/RuntimeService.cpp
@@ +931,5 @@
>                                       mSystemCharset);
>    }
>  
> +  rv = InitOSFileConstants();
> +  if (!NS_SUCCEEDED(rv)) {

if (NS_FAILED(rv))

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

Thanks, I just learnt something.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 635709 [details] [diff] [review]
Companion testsuite

Review of attachment 635709 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't review the actual test, just the build files.

::: dom/system/tests/Makefile.in
@@ +4,5 @@
> +VPATH            = @srcdir@
> +relativesrcdir   = dom/system/tests
> +
> +MODULE           = test_domsystem
> +DIRS = mochi

You don't need an entire subdirectory for mochitests.  Move the contents of ./mochi/ to this directory.

::: dom/system/tests/mochi/Makefile.in
@@ +7,5 @@
> +MODULE          = domsystem
> +_CHROME_TEST_FILES = \
> +	test_constants.xul \
> +	worker_constants.js \
> +         $(NULL)

No tabs necessary outside of rules.

@@ +9,5 @@
> +	test_constants.xul \
> +	worker_constants.js \
> +         $(NULL)
> +
> + include $(topsrcdir)/config/rules.mk

Nit: extra space?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Fixed, thanks - and thanks for the quick reviews.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

Same patch, except we heap-allocate the string instead of static-allocating it, to avoid any leak.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing path to libxul in OS.Constants

+ merging

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4f69c478fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9924488dd2

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b4f69c478fa
https://hg.mozilla.org/mozilla-central/rev/5d9924488dd2