1113471 - OS.Constants.Win.GENERIC_READ is wrong

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: OS.File, defect)

Description

[noitidart]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141211142524

Steps to reproduce:

OS.Constants.Win.GENERIC_READ is coming back as -2147483648

It shouldbe though 0x80000000
Which is 2147483648

Comment 1

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

The right way to fix this is to change the definition of GENERIC_READ in http://dxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.cpp to use `UINT_CONSTANT` instead of `INT_CONSTANT`.

Comment 2

[Manan Doshi]

I'd like to work on it.

Comment 3

[Manan Doshi]

Attachment: generic_read.patch

Made changes as suggested.

Comment 4

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

https://tbpl.mozilla.org/?tree=Try&rev=a7c86d92d965

Comment 5

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

Comment on attachment 8547138 [details] [diff] [review]
generic_read.patch

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

Looks good, thanks.
Let's test that this doesn't break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d61bb8c11e29

Also, can you use "r=yoric" at the end of your commit message?

Comment 6

[Manan Doshi]

How do I test it? (I am fairly new to this).

Comment 7

[Manan Doshi]

Attachment: generic_read.patch

Added r=yoric

Comment 8

[Manan Doshi]

forgot to add a needinfo

Comment 9

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

Comment on attachment 8550843 [details] [diff] [review]
generic_read.patch

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

Looks good to me.
Try results will show up here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e410317d26a

Comment 10

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

Tests look good. Are you ready to land this, Manan?

Comment 11

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

Well, I'm going to assume that he is.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows test_osfile_back.xul failures.
https://hg.mozilla.org/integration/fx-team/rev/c218112d84f7

https://treeherder.mozilla.org/logviewer.html#?job_id=2341144&repo=fx-team

Comment 14

[noitidart]

I don't understand how UINT_CONSTANT made the test_osfile_back.xul fail

Can someone please help me understand that, now that i got firefox built I was hoping to land a fix for this

Comment 15

[Chris Peterson [:cpeterson]]

David: do you know why changing INT_CONSTANT to UINT_CONSTANT broke the test_osfile_back.xul test on fxteam but not Try? From the test errors, it looks like Python choked when parsing ps' output:

14. 'c:/mozilla-build/python27/python -u ...' warnings 46m 30s 27ms
1637 10:43:16 INFO - ValueError: invalid literal for int() with base 10: 'ps:'
1651 10:43:16 INFO - ValueError: invalid literal for int() with base 10: 'ps:'
28235 11:00:05 INFO - 2066 INFO TEST-UNEXPECTED-FAIL | toolkit/components/osfile/tests/mochi/test_osfile_back.xul | Worker error TypeError: expected type int32_t, got 2147483648 - expected PASS
28291 11:05:06 INFO - 2067 INFO TEST-UNEXPECTED-FAIL | toolkit/components/osfile/tests/mochi/test_osfile_back.xul | Test timed out. - expected PASS

Comment 16

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

Apparently, GENERIC_READ is indeed supposed to be a DWORD, so a signed int. So INT_CONSTANT seems to be the right way to define it.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

DWORD is unsigned.

Comment 18

[Masatoshi Kimura [:emk]]

DWORD flags are intentionally defined as signed.
https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/toolkit/components/osfile/modules/osfile_win_back.jsm#104
INVALID?

Comment 19

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

OS.File is on its way out, closing bug.