739740 - libc constants for js-ctypes (Chrome Workers)

Status: RESOLVED FIXED

(Core :: js-ctypes, enhancement)

Description

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

Not quite sure in which component to place this bug.

Now that we (almost) have errno support in js-ctypes (see bug 684017), we still need to be able to compare error numbers with their actual definition in errno.h. As these values are defined as macros and are not portable among OSes, this requires some C/C++.

More generally, we need to export to JavaScript all the libc constants, including constants for |open|, |seek|, etc.

I will open another bug for (less portable) Windows-specific constants.

Comment 1

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

Attachment: Exporting libc constants to JS

Attaching a first draft. This module exports a global object |OS.Constants.libc|, with many (all?) of the important libc constants. I envision that this module can be later extended with additional constants as needed.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 609840 [details] [diff] [review]
Exporting libc constants to JS

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

::: toolkit/components/osfile/Makefile.in
@@ +55,5 @@
>  
>  EXPORTS_mozilla = OSFile.h \
>     $(NULL)
>  
> +CPPSRCS = OSFileConstants.cpp \

Please put that on a separate line.

::: toolkit/components/osfile/OSFileConstants.cpp
@@ +25,5 @@
> +  /**
> +   * JS value of the property. Any JS value will do.
> +   */
> +  jsval value;
> +  unsigned flags;

Considering the flags are always the same, you don't need this.

@@ +179,5 @@
> +  INT_CONSTANT(EROFS),
> +  INT_CONSTANT(ESPIPE),
> +  INT_CONSTANT(ETIMEDOUT),
> +  INT_CONSTANT(ETXTBSY),
> +  INT_CONSTANT(EWOULDBLOCK),

I don't think we want to be that exhaustive in the exposed constants. (esp. since each and every one of them adds a string and a relocation)

Comment 3

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

(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 609840 [details] [diff] [review]
> Exporting libc constants to JS
> 
> Review of attachment 609840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/Makefile.in
> @@ +55,5 @@
> >  
> >  EXPORTS_mozilla = OSFile.h \
> >     $(NULL)
> >  
> > +CPPSRCS = OSFileConstants.cpp \
> 
> Please put that on a separate line.
> 
> ::: toolkit/components/osfile/OSFileConstants.cpp
> @@ +25,5 @@
> > +  /**
> > +   * JS value of the property. Any JS value will do.
> > +   */
> > +  jsval value;
> > +  unsigned flags;
> 
> Considering the flags are always the same, you don't need this.

I initially intended to add some variety, but you are right.

> @@ +179,5 @@
> > +  INT_CONSTANT(EROFS),
> > +  INT_CONSTANT(ESPIPE),
> > +  INT_CONSTANT(ETIMEDOUT),
> > +  INT_CONSTANT(ETXTBSY),
> > +  INT_CONSTANT(EWOULDBLOCK),
> 
> I don't think we want to be that exhaustive in the exposed constants. (esp.
> since each and every one of them adds a string and a relocation)

My protocol was the following: read the manpage of every function I was exporting, and add all the errors that appear somewhere in that manpage. Now, if you think that these constants are useless, I can remove them, but do you have a good idea on which list would be reasonable?

Comment 4

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> My protocol was the following: read the manpage of every function I was
> exporting, and add all the errors that appear somewhere in that manpage.
> Now, if you think that these constants are useless, I can remove them, but
> do you have a good idea on which list would be reasonable?

I think it would be reasonable to only define those that are useful to us. If anyone requires more, they can 1. file a bug and/or 2. define the constant themselves.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 609840 [details] [diff] [review]
Exporting libc constants to JS

Fine with me (but of course don't take my feedback as overriding glandium's).

Comment 6

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

Attachment: Exporting libc constants to JS

Reduced the number of symbols involved to what seems a "reasonable" subset of libc (basically, the constants used by the functions I am using in OS.File tests atm). Does this sound like a reasonable set of constants?

Comment 7

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

Attachment: Building and exporting the module

Comment 8

[(dormant account)]

Comment on attachment 612554 [details] [diff] [review]
Exporting libc constants to JS

r+ for the general idea of compiling a string->int table.



SimplePropertySpec gLibcProperties[]  should be static or in anon name


I'm also not a good reviewer for nsIExporting interesting new bits.

If i was I would say:
I think IOSFileService.error_codes should be implemented by returning a JSObject with a lazy getter so when now does nsIOSFileService.error_codes.O_RDONLY, .error_codes returns an object that implements a lazy getter (via JS Resolve hook?) that then looks up the "O_RDONLY" property in a table or a generated switch statement(table is probably better, since that makes it easier to support enumeration).space

Perhaps one of khuey/bent/jst/bholley etc be a better reviewer?

Comment 9

[(dormant account)]

Comment on attachment 612555 [details] [diff] [review]
Building and exporting the module

Kyle got more makefile-foo than me

Comment 10

[(dormant account)]

David, I nominate dougt as a toolkit reviewer for file api. I think he implemented part of the nsIFile* stuff.

It is probably wise to disregard my lazyness suggestion in the above review.

Comment 11

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

Comment on attachment 612555 [details] [diff] [review]
Building and exporting the module

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

I don't really understand what you're trying to do here.  Why is there an XPCOM service involved at all?

Having dom/ include stuff from toolkit/ seems kind of tacky too.  Sticking this in dom/system seems reasonable to me.

Comment 12

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #11)
> Comment on attachment 612555 [details] [diff] [review]
> Building and exporting the module
> 
> Review of attachment 612555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really understand what you're trying to do here.  Why is there an
> XPCOM service involved at all?

Objective: export libc constants so that libraries based on js-ctypes can use them. This cannot be done in pure JS because constants differ between platforms, or from js-ctypes, because these constants are actually #define.

Well, the patch in dom/ ensures that chrome workers have access to this object. The xpcom service ensures that main thread can have access to it, too. I could also have automatically patched the global scope of chrome main thread contexts, but this less invasive technique was recommended to me a few months ago by someone on the jsapi team (can't remember who).

> Having dom/ include stuff from toolkit/ seems kind of tacky too.  Sticking
> this in dom/system seems reasonable to me.

Ok.

Comment 13

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

(In reply to Taras Glek (:taras) from comment #8)
> Comment on attachment 612554 [details] [diff] [review]
> Exporting libc constants to JS
> 
> r+ for the general idea of compiling a string->int table.
> 
> 
> 
> SimplePropertySpec gLibcProperties[]  should be static or in anon name

ok.

> I'm also not a good reviewer for nsIExporting interesting new bits.
> 
> If i was I would say:
> I think IOSFileService.error_codes should be implemented by returning a
> JSObject with a lazy getter so when now does
> nsIOSFileService.error_codes.O_RDONLY, .error_codes returns an object that
> implements a lazy getter (via JS Resolve hook?) that then looks up the
> "O_RDONLY" property in a table or a generated switch statement(table is
> probably better, since that makes it easier to support enumeration).space

This sounds feasible, but I am not sure what this change would bring. If we don't use a |switch| statement, this essentially means that we handle atomization and the hash table manually instead of letting JSAPI handling it. We might possibly save a little memory, but I fear that we will lose some JIT inlining opportunities, too. Plus code will be more complicated.


> 
> Perhaps one of khuey/bent/jst/bholley etc be a better reviewer?

Comment 14

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

Attachment: Exporting libc constants to JS

Moved to dom/system, as discussed, and removed the xpcom component for the time being. This version also introduces some of the Windows constants.

Comment 15

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

Attachment: Exporting libc constants to JS

Same patch, but without Windows stuff.

Comment 16

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

Comment on attachment 617430 [details] [diff] [review]
Exporting libc constants to JS

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

::: dom/system/Makefile.in
@@ +86,1 @@
>    $(NULL)

Please export OSFileConstants with namespacing.

e.g.

EXPORTS_NAMESPACES = mozilla

EXPORTS_mozilla = OSFileConstants.h

::: dom/system/OSFileConstants.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +
> +#include "fcntl.h"

Nit: one newline between the license block and the #include.

@@ +34,5 @@
> +  /**
> +   * JS value of the property. Any JS value will do.
> +   */
> +  jsval value;
> +} SimplePropertySpec;

Lets just reuse ConstantSpec from dom/bindings/Utils.h.

@@ +53,5 @@
> + */
> +#define PROP_END { NULL, JSVAL_VOID }
> +
> +
> +// Define placeholders for Android

Really?  Android is such a POS sometimes.

Why do we want to define these to wrong values rather than leaving them out entirely?

@@ +238,5 @@
> +                           NULL, NULL,
> +                           flags) ) {
> +      return false;
> +    }
> +  }

Conveniently, there's a DefineConstants in dom/bindings/Utils.cpp.  Lets expose that in Utils.h and use it here.

::: dom/system/OSFileConstants.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +
> +#ifndef mozilla_osfile_osfileconstants_h__

Nit: one newline between the license block and the ifndef.

@@ +5,5 @@
> +
> +#ifndef mozilla_osfile_osfileconstants_h__
> +#define mozilla_osfile_osfileconstants_h__
> +
> +#include "jsapi.h"

Please include jspubtd.h instead of jsapi.h here.

@@ +8,5 @@
> +
> +#include "jsapi.h"
> +
> +namespace mozilla {
> +namespace osfile {

Lets drop the second level of namespacing here, and just have mozilla::DefineOSFilePlatformConstants.

::: dom/workers/Makefile.in
@@ +59,5 @@
>  LOCAL_INCLUDES = \
>    -I$(topsrcdir)/content/base/src \
>    -I$(topsrcdir)/content/events/src \
>    -I$(topsrcdir)/dom/base \
> +  -I$(topsrcdir)/dom/system \

This should not be necessary.

Comment 17

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> > + */
> > +#define PROP_END { NULL, JSVAL_VOID }
> > +
> > +
> > +// Define placeholders for Android
> 
> Really?  Android is such a POS sometimes.
> 
> Why do we want to define these to wrong values rather than leaving them out
> entirely?

Well, the reasoning is the following. I suspect that future users of this library are not aware that these constants do not exist specifically on Android. As function |open| still takes an argument obtained by composing these constants (which is just ignored on Android), I suspect that users will try and compose these constants. If the constants are undefined, the result will be |undefined|, which, iirc, will cause a surprising and hard-to-read runtime error during the call to the C function.

Being myself the primary user for this function, it also saves me from having to write a distinct variant of the high-level file-opening functions specifically for Android.

> Conveniently, there's a DefineConstants in dom/bindings/Utils.cpp.  Lets
> expose that in Utils.h and use it here.

Ok, I will look at that.

> Lets drop the second level of namespacing here, and just have
> mozilla::DefineOSFilePlatformConstants.

Ok.

Thanks for the quick review.

Comment 18

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> Well, the reasoning is the following. I suspect that future users of this
> library are not aware that these constants do not exist specifically on
> Android. As function |open| still takes an argument obtained by composing
> these constants (which is just ignored on Android), I suspect that users
> will try and compose these constants. If the constants are undefined, the
> result will be |undefined|, which, iirc, will cause a surprising and
> hard-to-read runtime error during the call to the C function.

Why not either define the actual values or entirely leave these constants out? They kind of look like expensive ways to write 0755 and such, which we have been happily using in JS without defining constants so far.

Comment 19

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

Well, that was when 0755 was legal in JS, wasn't it?
Now, using octal in JS raises a warning or an error in strict mode.

Comment 20

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> Well, that was when 0755 was legal in JS, wasn't it?
> Now, using octal in JS raises a warning or an error in strict mode.

Then that's even more a reason *not* to put broken values on Android.

Comment 21

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

Not sure I understand. What would you suggest?

Comment 22

[Mike Hommey [:glandium]]

Setting real values instead of 0

Comment 23

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

You want me to put the Linux values even though they are meaningless on Android? I can do that, but what is the point?

Comment 24

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #23)
> You want me to put the Linux values even though they are meaningless on
> Android? I can do that, but what is the point?

S_IRGRP and friends? meaningless on Android? They have the same values and meaning on any unix system, OSX included.

Comment 25

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

I had the impression that the Android file system does not care about these constants. Am I wrong?

Comment 26

[Mike Hommey [:glandium]]

Except /sdcard, which is vfat, it does care about them. Regardless, all you'd achieve with 0 on every value is to create unreadable files and directories.

Comment 27

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

Ok, I will fix this.

Comment 28

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

Attachment: Exporting libc constants to JS

Comment 29

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

Comment on attachment 618199 [details] [diff] [review]
Exporting libc constants to JS

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

I'd like glandium to sign off on the Android stuff.

::: dom/system/OSFileConstants.cpp
@@ +183,5 @@
> +  if (JS_GetProperty(cx, aObject, aProperty, &val)
> +      && !JSVAL_IS_VOID(val)) {
> +    if (JSVAL_IS_OBJECT(val)) {
> +      return JSVAL_TO_OBJECT(val);
> +    } else {

}
else {

is the prevailing style here.

@@ +212,5 @@
> +  JSObject *objLibc;
> +  if (!(objLibc = GetOrCreateObjectProperty(cx, objConstants, "libc"))) {
> +    return false;
> +  }
> +  return dom::bindings::DefineConstants(cx, objLibc, gLibcProperties);

Is it intentional that we leave OS.Constants empty?

::: dom/system/OSFileConstants.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_osfile_osfileconstants_h__
> +#define mozilla_osfile_osfileconstants_h__

mozilla_osfileconstants_h__

::: dom/workers/WorkerScope.cpp
@@ +62,5 @@
>  #include "File.h"
>  #include "FileReaderSync.h"
>  #include "Location.h"
>  #include "Navigator.h"
> +#include "OSFileConstants.h"

mozilla/OSFileConstants.h ?

Comment 30

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

Attachment: Exporting libc constants to JS

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 619507 [details] [diff] [review]
Exporting libc constants to JS

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

::: dom/system/OSFileConstants.cpp
@@ +50,5 @@
> +#define S_IWOTH 2
> +#define S_IWUSR 128
> +#define S_IXOTH 1
> +#define S_IXGRP 8
> +#define S_IXUSR 64

Octal values would be more obvious to the eye. (S_IRGRP is 0040, S_IRUSR 0400, S_IROTH 0004, etc.)

@@ +120,5 @@
> +  INT_CONSTANT(W_OK),
> +  INT_CONSTANT(X_OK),
> +#endif // defined(F_OK)
> +
> +  // modes (not defined on Android)

"not defined on Android" not true anymore, right?

Comment 32

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

Attachment: Exporting libc constants to JS

Fixed. Thanks for the reviews!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

unable to find 'dom/bindings/Utils.cpp' for patching
1 out of 1 hunks FAILED -- saving rejects to file dom/bindings/Utils.cpp.rej
unable to find 'dom/bindings/Utils.h' for patching
1 out of 1 hunks FAILED -- saving rejects to file dom/bindings/Utils.h.rej

Comment 34

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

Attachment: Exporting libc constants to JS

Propagated recent file and namespace renamings.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fd22efe23d

Should this have tests?

Comment 36

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

Backed out due to bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd031e136ed

../../../dom/workers/WorkerScope.cpp: In function 'JSObject* mozilla::dom::workers::CreateDedicatedWorkerGlobalScope(JSContext*)':
../../../dom/workers/WorkerScope.cpp:992:42: error: 'DefineOSFileConstants' was not declared in this scope

Comment 37

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

Attachment: Exporting libc constants to JS

Headwall and apologies.

Comment 38

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

Attachment: Exporting libc constants to JS

Comment 39

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

(In reply to Ryan VanderMeulen from comment #35)
> Should this have tests?

Probably not, this is just a set of constants definitions, split away from bug 707679.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/685e5580a1f4

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/685e5580a1f4

Comment 42

[[e-mail]]

This doesn't build on VC9 because it doesn't have ELOOP, ELOOP, EOPNOTSUPP, EOVERFLOW, ETIMEDOUT or EWOULDBLOCK.

Comment 43

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

Ah, sorry, it was tested only on VC10. I will fix this.