996105 - String type check typo in nsWindowsRegKey::ReadStringValue()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Maks Naumov]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

nsWindowsRegKey.cpp 292

if (type != REG_SZ && type == REG_EXPAND_SZ && type == REG_MULTI_SZ)
return NS_ERROR_FAILURE;

This condition is always false.

Comment 1

[Benjamin Smedberg]

The condition is true if type == REG_SZ. But you are correct that it will fail for REG_EXPAND_SZ and REG_MULTI_SZ which is not what we want according to the comment. It also means that the REG_EXPAND_SZ code is completely untested.

I'm a little worried about that; without additional automated tests the only patch I'd want to take here is to remove all of the EXPAND_SZ and MULTI_SZ codepaths. We can fix them with test coverage.

http://hg.mozilla.org/mozilla-central/annotate/265d82091bce/xpcom/ds/nsWindowsRegKey.cpp#l292

Comment 2

[Anuj Agarwal [:anujagarwal464]]

I would like to work on this. Could you explain me what needs to be done here?

Comment 3

[Benjamin Smedberg]

Anuj, comment 1 has the link to the broken code. You need to build Firefox and fix the code I linked there.

There is some testing code at http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestWinReg.js but as far as I know it doesn't run automatically as part of our test suites. It would be awesome to make this test a real xpcshell test (in xpcom/tests/unit). See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests for instructions on xpcshell tests, as well as the other examples in the xpcom/tests/unit directory.

Comment 4

[Anuj Agarwal [:anujagarwal464]]

While writing xpcshell test, what conditions do I need to check 
http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestWinReg.js

Comment 5

[Benjamin Smedberg]

Basic sanity-check of the API:

* create a key structure in a spot that's normally writable (somewhere under HKCU). If it's already present because a previous test crashed or didn't clean up properly, clean it up first.
* test that the write* functions write stuff
* check that the valueCount/getValueName functions work for the values we just wrote
* check that the get* functions work for the values we just wrote.
* check that the get* functions fail with the right exception codes if we ask for the wrong type or if the value name doesn't exist at all
* check that creating/enumerating/deleting child keys works
* clean up

Comment 6

[pawel]

Hi,

I would like to work on this bug. Can you make an assignement?

Regards!

Comment 7

[Benjamin Smedberg]

Pawel, typically we don't assign bugs to new contributors until they have provided an initial patch. Please feel free to work on this!

Comment 8

[sajitk]

Attachment: 996105.patch

Please find attached unit tests for the registry code as mentioned in Comment 5

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 8664016 [details] [diff] [review]
996105.patch

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

Great! I've set the review flag on the patch to ensure that somebody looks at it :)

::: xpcom/tests/unit/xpcshell.ini
@@ +74,5 @@
>  # Bug 676998: test fails consistently on Android
>  fail-if = os == "android"
>  [test_file_renameTo.js]
>  [test_notxpcom_scriptable.js]
> +skip-if = os != "win"

This line affects the test above it, so it should shift down one line.

Comment 10

[sajitk]

Attachment: 996105.patch

Minor update

Comment 11

[sajitk]

Attachment: 996105.patch

Commented out some tests which were Failing, possibly because of the bug mentioned here.
Please let me know of any feedback, I will work on finding out more about the test failures.

Comment 13

[sajitk]

Attachment: 996105.patch

Comment 14

[Benjamin Smedberg]

Comment on attachment 8666543 [details] [diff] [review]
996105.patch

This looks good. Thank you.

Comment 15

[sajitk]

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

Comment 16

[sajitk]

Could you please assign this bug to me?
Thanks

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7077589b174c

Comment 18

[Nigel Babu [:nigelb]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/915c182cf521 for XPCShell bustage on Linux and OS X that looks like this https://treeherder.mozilla.org/logviewer.html#?job_id=15546649&repo=mozilla-inbound

Comment 19

[sajitk]

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

Comment 20

[sajitk]

Attachment: 996105.patch

Comment 21

[Nigel Babu [:nigelb]]

Please add r=reviewer in the commit message in the future.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba7db03684c

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ba7db03684c