1136110 - Define OS.Constants.Sys.bits

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

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

We need a way for add-ons to determine whether they are running on a 32-bits or 64-bits platform. For this purpose, we should extend the existing object OS.Constants.Sys with this information.

The code lives in file OSFileConstants.cpp, which may be found here: https://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.cpp?from=OSFileConstants.cpp#913-918

The objective of this bug is to patch function `DefineOSFileConstants` to add to `objSys` a field "bits", whose value is 32 on a 32-bits platform and 64 on a 64-bits platform.

Comment 1

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

Implementation note: we may use `#if defined(HAVE_64BIT_BUILD)` to find out whether we are building for 64 bits.

Comment 2

[Vaibhav Bhosale]

Hey David,
Can I work on this bug?
I looked through the file OSFileConstants.cpp in the repository I have. But I fail to understand what you mean mean by adding a field to the object. Can you give any hint with regards to this?
Thank You!

Regards,
Vaibhav

Comment 3

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

Hi Vaibhav, and thanks for picking this bug.

OSFileConstants.cpp defines in C++ a JavaScript object called `OS`. This object is defined in C++ because it contains information that is not available to JavaScript. This object contains a field `Constants`, which itself is an object contains a field `Sys`, which itself is an object containing a few fields such as `Name` (the name of the operating system) or `DEBUG` (which is `true` if and only if we are running a debug version of Firefox).

For instance, you can find the following snippet:

1.  JS::Rooted<JS::Value> valDebug(cx, JSVAL_TRUE);
2.  if (!JS_SetProperty(cx, objSys, "DEBUG", valDebug)) {
3.    return false;
4.  }

This extract defines the field DEBUG of OS.Constants.Sys. In JavaScript, this would be written
  OS.Constants.Sys.DEBUG = true

Let's walk through it line by line

// Define `valDebug`, holding the JavaScript value `true`
1.  JS::Rooted<JS::Value> valDebug(cx, JSVAL_TRUE);

// Call JS_SetProperty to define the field called "DEBUG"
// in object `objSys`, and set it to value `valDebug`.
2.  if (!JS_SetProperty(cx, objSys, "DEBUG", valDebug)) {

// If the call failed for some reason, propagate the error.
3.    return false;
4.  }


Does this help?

Comment 4

[Vaibhav Bhosale]

Thanks a lot David.
Do we need to implement this add-on for the debug version only?
If yes, I guess the snippet would be:

#if defined(DEBUG)
  JS::Rooted<JS::Value> valDebug(cx, JSVAL_TRUE);
  #if defined(HAVE_64BIT_BUILD){//Check if it is a 64 bit machine
    if (!JS_SetProperty(cx, objSys, "DEBUG", valDebug, "bits", 64)) {
    return false;
  }
  }
  #else{
    if (!JS_SetProperty(cx, objSys, "DEBUG", valDebug, "bits", 32)) {
    return false;
  }
  }
  #endif //defined(HAVE_64BIT_BUILD)
#endif

Please correct me if I am wrong.
Also please tell me how do I make a test run of this to know if it actually works; and also on how do I submit the patch.

Thank You!

Regards,
Vaibhav

Comment 5

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

No, this patch should be for all versions, not just debug.

To check whether it works, the first thing to do is to build the code, using
  ./mach build

Now, you should quickly realize that your code doesn't build (at least if you move it out of the `#if defined(DEBUG)` block) :)

To fix this, take a look at the documentation of JS_SetProperty here:
 https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_SetProperty
To create a JS::Rooted<JS::Value> from an integer, use macro `UINT_TO_JSVAL`.

Once your code builds, we'll discuss how to write a unit test for that property.

Don't hesitate to come over IRC if you have any question. We are on irc.mozilla.org, channel #introduction, or if you do not have a IRC client, http://client00.chat.mibbit.com/?server=irc.mozilla.org&channel=%23introduction&nick=Vaibhav

Finally, we have put together guidelines on patch submission here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 6

[Vaibhav Bhosale]

Hey David,
I made the changes you suggested and when I build the code, it shows successful.
I added the following snippet:
#if defined(HAVE_64BIT_BUILD)
  JS::Rooted<JS::Value> osversion(cx, INT_TO_JSVAL(64));
  if (!JS_SetProperty(cx, objSys, "bits", osversion)) {
    return false;
  }
#else
  JS::Rooted<JS::Value> osversion(cx, INT_TO_JSVAL(32));
  if (!JS_SetProperty(cx, objSys, "bits", osversion)) {
    return false;
  }  
#endif //defined (HAVE_64BIT_BUILD)

Is this what you were expecting? If yes, what should be my further steps?
Else, please explain me the mistake.

Thank You!

Regards,
Vaibhav

Comment 7

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

At a first glance, this looks good, although I'll have a few minor remarks.
Now, the next step is to prepare a patch.

1. If you haven't done so yet, run
  ./mach mercurial-setup
you only need to do this once.

2. Commit your changes and give them a name by doing
  hg commit -m "Bug 1136110 - Define OS.Constants.Sys.bits;r=yoric"

3. Export your patch to a file by doing
  hg export tip > /tmp/mypatch.diff

4. Attach the patch to this bug and use the "feedback" field to put a "?" and my nickname (":Yoric").

I will then see the patch in the feedback/review interface, plus it will show up in my todo list, and I'll be able to give you line-by-line comments.

Comment 8

[Vaibhav Bhosale]

Attachment: mypatch.diff

Comment 9

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

Comment on attachment 8569851 [details] [diff] [review]
mypatch.diff

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

Looks mostly good. Can make the improvements below?

Once you have done this, you will need to commit the new patch and upload it again. One way to update your patch directly is
  hg commit --amend

::: dom/system/OSFileConstants.cpp
@@ +917,5 @@
>    }
>  #endif
>  
> +#if defined(HAVE_64BIT_BUILD)
> +  JS::Rooted<JS::Value> osversion(cx, INT_TO_JSVAL(64));

Nit: It's not the version of the OS, it's the number of bits, so I would call this `valBits`.

@@ +925,5 @@
> +#else
> +  JS::Rooted<JS::Value> osversion(cx, INT_TO_JSVAL(32));
> +  if (!JS_SetProperty(cx, objSys, "bits", osversion)) {
> +    return false;
> +  }  

Nit: Please avoid whitespace at the end of your line.

@@ +926,5 @@
> +  JS::Rooted<JS::Value> osversion(cx, INT_TO_JSVAL(32));
> +  if (!JS_SetProperty(cx, objSys, "bits", osversion)) {
> +    return false;
> +  }  
> +#endif //defined (HAVE_64BIT_BUILD)

You can factor this a little bit more.
#if defined(HAVE_64BIT_BUILD)
  JS::Rooted<> ...
#else
  JS::Rooted<> ...
#endif // ...
  if (!JS_SetProperty(...)) {
    ...
  }

Comment 10

[Vaibhav Bhosale]

Attachment: mypatch.diff

Comment 11

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

Comment on attachment 8569859 [details] [diff] [review]
mypatch.diff

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

That looks good.
The next step is to write a test.

Since there is no good way to check the information from JavaScript, the best you can do is to patch dom/system/tests/worker_constants.js to check that OS.Constants.Sys.bits is either 32 or 64.
To run the test
  ./mach mochitest dom/system/tests/test_constants.xul

Comment 12

[Vaibhav Bhosale]

Thanks David
On running the test, all looked fine; just at the end there were a few errors:
38 INFO SimpleTest FINISHED
###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
TEST-INFO | Main app process: exit 0

Can you please clarify these?
Or should I give you the entire dump?

Comment 13

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

Don't worry about these, they are unrelated to your patch.

Comment 14

[Vaibhav Bhosale]

So, what should be my further steps?

Comment 15

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

Patch dom/system/tests/worker_constants.js as described in comment 11 and attach the patch for review.

Comment 16

[Vaibhav Bhosale]

Attachment: test_constants.xul

Comment 17

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

Ah, you didn't attach a patch, but the entire file. Could you just attach a patch? Since your total patch is pretty small, let's use
  hg commit --amend
to add this to the patch you have been working on so far.

Comment 18

[Vaibhav Bhosale]

But there were no changes made in that file. So what should I submit?
I apologize for such silly questions, but I am confused here. Please explain.

Thanks!

Comment 19

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

Can you change dom/system/tests/worker_constants.js to check that OS.Constants.Sys.bits is either 32 or 64?

Comment 20

[Vaibhav Bhosale]

On checking for the particular value, what action should be taken?

Comment 21

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

Use `is(OS.Constants.Sys.bits == 32 || OS.Constants.sys.bits == 64, "OS.Constants.Sys.bits is either 32 or 64")`.

Comment 22

[Vaibhav Bhosale]

Does the value of OS.Constants.Sys.bits being 32 or 64 suffice or we need to give the specific value?
And does this value need to be sent as a message in the self.onmessage function?
Thanks!

Comment 23

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

For the moment, we have no way to check whether the platform is 32 bits or 64 bits from JavaScript, so this will have to do.

You don't need to send any message in self.onmessage. Just do something comparable to `test_name()`.

Comment 24

[Vaibhav Bhosale]

Would this do?

function test_bits(){
  is(OS.Constants.Sys.bits == 32 , "OS.Constants.Sys.bits is 32");
  is(OS.Constants.Sys.bits == 64 , "OS.Constants.Sys.bits is 64");
}

Also, where will this function be called?
Thanks!

Comment 25

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

Whenever you call `is(...)`, this causes a test failure if the condition is not verified, so this version of `test_bits` would always fail.

Oh, and I realize that I misled you. `is` checks for equality, what you want here is `ok(some condition, some text)`.

And you should add a call to the function in `self.onmessage`.

Note that this test is by no mean representative of our test suites. Most our tests are better written than this one. My fault, and that of our test harness that doesn't offer good support for testing multi-threaded JavaScript.

Comment 26

[Vaibhav Bhosale]

Hey David,
I am unable to combine the two patches together. Can you please guide me with this?
Thank You!

Comment 27

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

To combine your last two patches, run
 hg histedit tip^

`histedit` means that you want to edit the history, which is used for combining two patches. `tip` means "latest patch", `tip^` is the patch before that, `tip^^` two patches before, etc.

This will open an editor showing some information your latest two patches. To edit history, you need to change the first word on the line, which is "pick" by default. For your *latest* patch, replace that word with "fold", then save the file and exit your editor.

Comment 28

[Vaibhav Bhosale]

Attachment: mypatch.diff

Comment 29

[Vaibhav Bhosale]

Hey David,
Can you please go through the latest patch I have submitted? Please let me know if I have made any mistake.
Thank You!

Comment 30

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c444cf694902

Comment 31

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

Comment on attachment 8570850 [details] [diff] [review]
mypatch.diff

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

This looks good to me, thanks.
Let's run this through the unit tests and hopefully land this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=709ce86ee3ec

Comment 32

[Vaibhav Bhosale]

Hey David,
Thanks for all the guidance.
I am unable to understand which unit tests you are trying to mention.
Also what is the link about?

Comment 33

[Manish Goregaokar [:manishearth]]

714 INFO TEST-UNEXPECTED-FAIL | dom/system/tests/test_constants.xul | TypeError: OS.Constants.sys is undefined

Comment 34

[noitidart]

captial on the s in sys so OS.Constants.Sys

also i saw someone doing a OS.Constants.Sys.bits i dont see that in Sys i only see OS.Constants.Sys.Name and OS.Constants.Sys.umask

on win8.1 umask was 0 for me and on ubuntu 14.04 umask was 2 for me

Comment 35

[noitidart]

Is this an ok way to test for 64bit system?

let numbits = ctypes.int.ptr.size * 8;
let platCode = numbits === 32 ? 'i686' : 'x86_64';

What are other values for platCode other then i686 and x86_64? just 64?

Comment 36

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

(In reply to Vaibhav Bhosale from comment #32)
> Hey David,
> Thanks for all the guidance.
> I am unable to understand which unit tests you are trying to mention.
> Also what is the link about?

So, I launched the unit tests, to make sure that 1/ the test you have written passes; 2/ we don't break any other test. That's the link I pasted in comment 31. As mentioned by manishearth in comment 33, your test doesn't pass, because of capitalization error.

Also, notidart's suggestion in comment 35 is good. We can actually check whether `OS.Constants.Sys.bits` is equal to `ctypes.int.ptr.size * 8`, which is the number of bits in a pointer to an integer. To test for equality, use `is(...)` instead of `ok(...)`.

Comment 37

[noitidart]

Cool :D credit goes to the guy i stole it from :P https://github.com/mozilla/deuxdrop/blob/2404f3fccaf762dfb935ee1563f68ca2c61d32b4/clients/addon/lib/nacl.js#L68

Comment 38

[Vaibhav Bhosale]

Attachment: mypatch.diff

This is the patch after making changes as suggested by notidart

Comment 39

[Manish Goregaokar [:manishearth]]

Comment on attachment 8574621 [details] [diff] [review]
mypatch.diff

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

::: dom/system/tests/worker_constants.js
@@ +77,5 @@
> +}
> +
> +// Check if the value of OS.Constants.Sys.bits is 32 or 64
> +function test_bits(){
> +  is(OS.Constants.Sys.bits == ctypes.int.ptr.size *, "OS.Constants.Sys.bits is either 32 or 64");

Shouldn't this be `ctypes.int.ptr.size * 8`?

Comment 40

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

Comment on attachment 8574621 [details] [diff] [review]
mypatch.diff

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

Manish is right. Have you tested your change?

Comment 41

[Vaibhav Bhosale]

Attachment: mypatch.diff

Comment 42

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

Comment on attachment 8575245 [details] [diff] [review]
mypatch.diff

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

This should work.

Comment 43

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

Let's look at the results of the unit tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=895220fa1448

Comment 44

[Vaibhav Bhosale]

Hey David,
Can you help me understand the results of the tests?
I see a few have failed. What should I do now?
Thanks!

Comment 45

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

The important part is the error message here:
https://treeherder.mozilla.org/logviewer.html#?job_id=5712141&repo=try

dom/system/tests/test_constants.xul | undefined assertion name - got false, expected OS.Constants.Sys.bits is either 32 or 64

To fix this, use `ok` instead of `is`.

Comment 46

[Vaibhav Bhosale]

Attachment: mypatch.diff

Comment 47

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

Comment on attachment 8580102 [details] [diff] [review]
mypatch.diff

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

Looks good.
Let's look at the unit tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=140ebabb373c

Comment 48

[Vaibhav Bhosale]

Hey David,
If I am not wrong all the tests have been passed as it shows "Failure summary is empty for all the tests".
Do I need to do anything else?
Is this bug ready to land?
Thanks

Comment 49

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

Attachment: Folded patch

Folding the various parts of the patch into a single patch.

Comment 50

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

Now, let's land this.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

umm...
https://treeherder.mozilla.org/logviewer.html#?job_id=5856661&repo=try

856 INFO TEST-UNEXPECTED-FAIL | dom/system/tests/test_constants.xul | OS.Constants.Sys.bits is either 32 or 64 - expected PASS

Comment 52

[Vaibhav Bhosale]

Hey David,
For the above warning what needs to be done?

Comment 53

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

Comment on attachment 8585351 [details] [diff] [review]
Folded patch

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

::: dom/system/tests/worker_constants.js
@@ +77,5 @@
>  }
> +
> +// Check if the value of OS.Constants.Sys.bits is 32 or 64
> +function test_bits(){
> +  ok(OS.Constants.Sys.bits == ctypes.int.ptr.size * 8, "OS.Constants.Sys.bits is either 32 or 64");

Can you replace this with
  `is(OS.Constants.Sys.bits, ctypes.int.ptr.size * 8, ...)`
?
This will give us more info about what's going wrong.

Comment 54

[Abhishek Singh Kushwah]

Hey,

I am new to mozilla is that bug still open?

If yes, whatPDT 's the status?

Comment 55

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

Vaibhav, see comment 52.

Comment 56

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

Hi Abhishek, Vaibhav is already working on the bug.

Comment 57

[Vaibhav Bhosale]

Attachment: patch.txt

Comment 58

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

Comment on attachment 8591203 [details] [diff] [review]
patch.txt

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

::: dom/system/tests/worker_constants.js
@@ +77,5 @@
>  }
> +
> +// Check if the value of OS.Constants.Sys.bits is 32 or 64
> +function test_bits(){
> +  is(OS.Constants.Sys.bits == ctypes.int.ptr.size * 8, "OS.Constants.Sys.bits is either 32 or 64");

Not `==`.

Comment 59

[Vaibhav Bhosale]

Attachment: patch.txt

Oops..
I am sorry. Didn't see it completely.

Comment 60

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

Comment on attachment 8591209 [details] [diff] [review]
patch.txt

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

Looks good.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6be22ae0746

Comment 61

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

Let's try to land this.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5695ef8fc94f

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5695ef8fc94f