750177 - Make OS.Constants lazy?

Status: RESOLVED WONTFIX

(Core :: js-ctypes, enhancement)

Description

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

Currently, |OS.Constants.libc| and under windows |OS.Constants.win| are initialized eagerly whenever a chrome worker is opened, regardless of whether they are used.

We could turn these objects into lazy getters.

Comment 1

[Amod [:AMoz]]

Sir, I would like to contribute to this bug. I would be highly obliged if u assign the bug to me..

Comment 2

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

For reference:
- about getters in JavaScript https://developer.mozilla.org/en/JavaScript/Guide/Working_with_Objects#Defining_Getters_and_Setters
- about getters in JSAPI https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Cookbook#Defining_a_property_with_a_getter_and_setter

Comment 3

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

(In reply to Amod from comment #1)
> Sir, I would like to contribute to this bug. I would be highly obliged if u
> assign the bug to me..

Here you go. Don't hesitate to contact me on IRC if you have any question.

Comment 4

[Amod [:AMoz]]

Sure...I will get back soon after my JS getters and setters...I have decided to shift to Linux..so downloading the source code..

Comment 5

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

Resetting ownership of this bug in the meantime.

Comment 6

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

Any news?

Comment 7

[Sankha Narayan Guria [:sankha]]

I talked to a few people on the IRC, and I think I will need to use resolve hooks to fix this. Its my first time with resolve hooks and it might take some time before I probably get it right.

Comment 8

[Sankha Narayan Guria [:sankha]]

Attachment: WIP patch

This is just a WIP patch. Can you tell me whether I am going in the right direction? I am creating a function to return the values for the first time, and in that function I'll initialize the constants for future use.

Comment 9

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

I will try and review this today. If I don't, this will have to wait about two weeks, so please accept my apologies for the delay.

Comment 10

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

Comment on attachment 725246 [details] [diff] [review]
WIP patch

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

Good start. However, what you are doing would define something along the lines of:

OS.Constants.libc = function libc() {
  OS.Constants.libc.CONSTANT_1 = VALUE_1;
  OS.Constants.libc.CONSTANT_2 = VALUE_2;
  // ...
}

::: dom/system/OSFileConstants.cpp
@@ +646,5 @@
> +  
> +  // now initialize the objects for future calls
> +  JSObject *objLibc;
> +  if (!(objLibc = GetOrCreateObjectProperty(cx, objConstants, "libc"))) {
> +    return false;

Instead of doing |GetOrCreateObjectProperty|, you should:
- create a new object |objLibc| using https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_NewObject
- use |objLibc| to overwrite the existing property libc using https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_DefineProperty

@@ +684,5 @@
>    }
>  
>    // Build OS.Constants.libc
>  
> +  JS_DefineFunction(cx, objConstants, "libc", lazyLibcGetter, 0, 0);

Instead of defining |libc| as a function, you should define it as a getter - see https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_DefineProperty

Comment 11

[Sankha Narayan Guria [:sankha]]

Attachment: WIP patch v2

I have made the suggested changes. But I am getting these build errors:

0:40.27 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In function ‘bool mozilla::DefineOSFileConstants(JSContext*, JSObject*)’:
 0:40.27 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:691:90: error: could not convert ‘0’ from ‘int’ to ‘jsval {aka JS::Value}’
 0:40.37 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In function ‘bool mozilla::lazyLibcGetter(JSContext*, unsigned int, jsval*)’:
 0:40.37 /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:658:1: error: control reaches end of non-void function [-Werror=return-type]

Comment 12

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

(In reply to Sankha Narayan Guria [:sankha93] from comment #11)
> Created attachment 729678 [details] [diff] [review]
> WIP patch v2
> 
> I have made the suggested changes. But I am getting these build errors:
> 
> 0:40.27
> /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In
> function ‘bool mozilla::DefineOSFileConstants(JSContext*, JSObject*)’:
>  0:40.27
> /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:691:
> 90: error: could not convert ‘0’ from ‘int’ to ‘jsval {aka JS::Value}’

This `0` is actually the |NULL| you pass as fourth argument. You should pass a jsval. Since you are actually not going to use that jsval, |JSVAL_VOID| (i.e. |undefined|) should do the trick.

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Cookbook?redirectlocale=en-US&redirectslug=SpiderMonkey%2FJSAPI_Phrasebook#Defining_a_read-only_property_with_only_a_getter

>  0:40.37
> /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp: In
> function ‘bool mozilla::lazyLibcGetter(JSContext*, unsigned int, jsval*)’:
>  0:40.37
> /home/sankha/oss/nightly/mozilla-central/dom/system/OSFileConstants.cpp:658:
> 1: error: control reaches end of non-void function [-Werror=return-type]

You forgot to return a boolean from that function. It should be |true| in case of success, |false| in case of error.

Comment 13

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

Working patch. All tests are passing on machine.

Comment 14

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

This patch lazy initializes both |OS.Constants.libc| and |OS.Constants.Win|.

Comment 15

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

Comment on attachment 730520 [details] [diff] [review]
patch v2

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

That looks good.

Now, I would like two things:
- confirmation from khuey that we are producing something useful;
- a test that ensures that OS.Constants.libc is defined, initially as a getter, and then as an object
- a test that ensures that OS.Constants.Win is defined only under Windows
- a test that ensures that, if OS.Constants.Win is defined, it is defined as a getter, and then as an object.

To determine whether a property is defined as a getter, you may use |Object.getOwnPropertyDescriptor|.

::: dom/system/OSFileConstants.cpp
@@ +652,5 @@
> +  }
> +  if(!JS_DefineProperty(cx, objConstants, "libc", OBJECT_TO_JSVAL(objLibc), NULL, NULL, JSPROP_READONLY)) {
> +    return false;
> +  }
> +  vp.setObject(*objLibc);

That's the first time I see |vp.setObject|. I suspect that the API has changed while I was looking away. Did you have confirmation from someone on the JSAPI team that this is the right way to do things?

@@ +661,5 @@
> + * Function to lazy initialize |OS.Constants.Win|
> + */
> +JSBool lazyWinGetter(JSContext *cx, JSHandleObject objConstants, JSHandleId id, JSMutableHandleValue vp)
> +{
> +#if defined(XP_WIN)

You should move this |#if defined|/|#endif| around the function.

Comment 16

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

Comment on attachment 730520 [details] [diff] [review]
patch v2

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

Replace NULL with nullptr everywhere.

This isn't a problem since you're just defining a property, but for the future you should be aware that lazyFooGetter can be invoked on objects other than OS.

I'm giving an r+ despite the comments because I trust Yoric to ensure that they are addressed before checking this in ;-)

::: dom/system/OSFileConstants.cpp
@@ +645,5 @@
> +  // initialize |OS.Constants.libc| for future calls
> +  JSObject *objLibc;
> +  if (!(objLibc = JS_NewObject(cx, NULL, NULL, objConstants))) {
> +    return false;
> +  }

Please add newlines between if blocks.

if (foo) {
  doThing();
}

if (bar) {
  doOtherThing();
}

etc

Comment 17

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

sankha93, could you apply khuey's review?

Comment 18

[Sankha Narayan Guria [:sankha]]

Attachment: Patch with XPCShell test

I have made the changes as khuey said. I also shifted the tests to XPC shell tests. But somehow I am having problems with executing them. Maybe I am missing something. Can you please check?

Comment 19

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

Comment on attachment 737484 [details] [diff] [review]
Patch with XPCShell test

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

Looks close to completion.

::: dom/system/tests/unit/test_lazyConstants.js
@@ +4,5 @@
> +function run_test() {
> +  Components.classes["@mozilla.org/net/osfileconstantsservice;1"].
> +    getService(Components.interfaces.nsIOSFileConstantsService).
> +    init();
> +  Components.utils.import("resource://gre/modules/ctypes.jsm");

I don't think you need to import ctypes.jsm

@@ +10,5 @@
> +}
> +
> +function test_lazyLibc() {
> +  var desc = Object.getOwnPropertyDescriptor(OS.Constants, "libc");
> +  isnot(undefined, desc.get, "OS.Constants.libc is defined as a getter function");

At this point, you should probably force evaluation of OS.Constants.libc.

::: dom/system/tests/unit/xpcshell.ini
@@ +1,2 @@
> +[DEFAULT]
> +head = head_channels.js head_cache.js

Your patch doesn't contain head_channels.js or head_cache.js. If you do not need these files, remove them from the head. If you need these files, could you please add them to the patch?

Comment 20

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

jwalden suggests that the properties should also have JSPROP_NATIVE_ACCESSORS

Comment 21

[Sankha Narayan Guria [:sankha]]

Attachment: patch without tests

This patch is just the changes in OSFileConstants.cpp. I am having trouble with the tests, will be doing something about it.

Comment 22

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

Comment on attachment 756909 [details] [diff] [review]
patch without tests

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

A review won't be very useful until we get the tests to pass.

Comment 23

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

Sankha, did you make any progress on that bug?

Comment 24

[Sankha Narayan Guria [:sankha]]

Yes, I will be uploading a patch within 2 days.

Comment 25

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

What's up, sankha93?

Comment 26

[Sankha Narayan Guria [:sankha]]

I am sorry, I may not have time to work on this at the moment.

Comment 27

[vinayakagarwal6996]

Is this bus still available to solve? Can I take it up?

Comment 28

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

Thanks for offering to help. That bug doesn't sound terribly useful these days, so I'm going to close it.