Closed Bug 996105 Opened 7 years ago Closed 6 years ago
String type check typo in ns
Windows Reg Key::Read String Value()
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: [firstname.lastname@example.org][lang=c++][good next bug] Requires windows
I would like to work on this. Could you explain me what needs to be done here?
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.
While writing xpcshell test, what conditions do I need to check http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestWinReg.js
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
Whiteboard: [email@example.com][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!
Pawel, typically we don't assign bugs to new contributors until they have provided an initial patch. Please feel free to work on this!
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.
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 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
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
Please add r=reviewer in the commit message in the future.
You need to log in before you can comment on or make changes to this bug.