Closed Bug 996105 Opened 6 years ago Closed 4 years ago

String type check typo in nsWindowsRegKey::ReadStringValue()

Categories

(Core :: XPCOM, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: maksqwe1, Assigned: sajitk, Mentored)

References

Details

(Whiteboard: [lang=c++][good next bug] Requires windows)

Attachments

(1 file, 4 obsolete files)

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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][good next bug] Requires windows
I would like to work on this. Could you explain me what needs to be done here?
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
While writing xpcshell test, what conditions do I need to check 
http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestWinReg.js
Flags: needinfo?(benjamin)
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
Flags: needinfo?(benjamin)
Mentor: benjamin
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][good next bug] Requires windows → [lang=c++][good next bug] Requires windows
Hi,

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

Regards!
Flags: needinfo?(benjamin)
Pawel, typically we don't assign bugs to new contributors until they have provided an initial patch. Please feel free to work on this!
Flags: needinfo?(benjamin)
Attached patch 996105.patch (obsolete) — Splinter Review
Please find attached unit tests for the registry code as mentioned in Comment 5
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.
Attachment #8664016 - Flags: review?(benjamin)
Attached patch 996105.patch (obsolete) — Splinter Review
Minor update
Attachment #8664016 - Attachment is obsolete: true
Attachment #8664016 - Flags: review?(benjamin)
Attachment #8664239 - Flags: review?(benjamin)
Attached patch 996105.patch (obsolete) — Splinter Review
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.
Attachment #8664239 - Attachment is obsolete: true
Attachment #8664239 - Flags: review?(benjamin)
Attachment #8664394 - Flags: review?(benjamin)
Duplicate of this bug: 1208899
Attached patch 996105.patch (obsolete) — Splinter Review
Attachment #8664394 - Attachment is obsolete: true
Attachment #8664394 - Flags: review?(benjamin)
Attachment #8666543 - Flags: review?(benjamin)
Comment on attachment 8666543 [details] [diff] [review]
996105.patch

This looks good. Thank you.
Attachment #8666543 - Flags: review?(benjamin) → review+
Could you please assign this bug to me?
Thanks
Flags: needinfo?(benjamin)
Assignee: nobody → sajitk
Flags: needinfo?(benjamin)
Keywords: checkin-needed
Attached patch 996105.patchSplinter Review
Attachment #8666543 - Attachment is obsolete: true
Keywords: checkin-needed
Please add r=reviewer in the commit message in the future.
https://hg.mozilla.org/mozilla-central/rev/9ba7db03684c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.