While working on bug 766799, I noticed that one xpcshell test was failing only on Mac, gdb confirmed the value was correct in the cpp side, but when passed to js false was becoming true. After debugging into the darwin xpcstubs I noticed the found value was indeed not a false bool as expected. This happens when bool is not in the first 5 integer params, since those are stored in integer registers, while from the 6th on they are passed on the stack, that is exactly my case: void onVisit(in nsIURI aURI, in long long aVisitID, in PRTime aTime, in long long aSessionID, in long long aReferringID, in unsigned long aTransitionType, in ACString aGUID, in boolean aHidden); I finally noticed something was handled differently in the Win64 xpcstubs, specifically this fix by Kato-san https://hg.mozilla.org/mozilla-central/rev/8c82de08425d Ans looks like here we fall into the same case, indeed doing the double cast properly handles the param as false. As Kato-san pointed, out this is compiler dependent, indeed on Linux doesn't happen, though to be on the safe side, since the fix is harmless, we should do this also on xptcstubs_x86_64_solaris and xptcstubs_x86_64_linux. I'll try to put together a test, I think we have an harness to check validity of xpcom stubs.

checkpointing the patch, now looking into the tests.

So I found this TestXPTCInvoke.cpp, that compiles as a simple program, or better I can't see where it's compiled cause there is no reference to its makefile... So maybe it is just not compiled? It also never checks output params values, even if that would be simple to do and a nice addition. Btw, I think it's worth to convert this to a cpp unit test, I'm not yet sure if this will be good for my purposes but could make sense to run tests instead of having them just lying around. Let's see where I end.

this converts TestXPTCInvoke.cpp to a cpp unit test, I also added a couple tests on bools, but I still have to verify if they catch the issue.

interestingly enough, my test is random failing the bools tests on Windows too, when I pass all true, I get "1, 1, 112, 1, 188, 1, 1, 1, 1, 1" and sometimes a zero. Btw, I'll first of all fix it to build across all platforms.

now, funny times. The test compiles and runs. https://tbpl.mozilla.org/?tree=Try&rev=9e3503ce048d The problem is that I have this random failure on Win, and failures on Mac. This is an invoke with all true arguments: TEST-INFO | (/Users/mbonardo/mozilla/mozilla-inbound/xpcom/reflect/xptcall/tests/TestXPTCInvoke.cpp) |1, 1, 0, 1, 0, 1, 1, 1, 1, 1

Found a bug in the test. Unfortunately now the test passes, so it's not good for my purpose.

Provided at this point I did the test conversion for the sake of it... any idea how I could modify the test to also go through the stubs, provided it's possible. Or which other test is covering stubs (apart those bat files spread around in md/tests).

I guess I should look into bug 684327

note to myself, I should likely take inspiration from test_params and write a test_manyParams, see: https://hg.mozilla.org/mozilla-central/rev/3d4cbdd72c03

I'm not sure what information you need... TextXPTCInvoke has been dead for years, as far as I know.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #11) > I'm not sure what information you need... TextXPTCInvoke has been dead for > years, as far as I know. yeah I figured that, I hope you don't mind I brought it back to life. I'll make a new test next week that should be good for this bug.

https://tbpl.mozilla.org/?tree=Try&rev=69edf93770b3 I added a test for many bool params to test_params.js, unfortunately it passes. Either the issue happens in specific states where the bools upper bits are dirty and in the test this doesn't happen, or this happens only when invoking a js implementation from cpp (the test invokes js from js and cpp from js)... If this still fails, I'll try to compare the call stacks to catch eventual differences I missed. I have no further ideas.

I have some problem completing a test that can verify this, basically the existing tests are all js->cpp, the backtrace is different since I see a NS_InvokeByIndex_P going directly into the stub, while in the bogus case I see nsNavHistory::NotifyOnVisit entering the stub, so my assumption is that the bug happens when we enter from cpp (maybe it does a malloc rather than a calloc). I have a cpp version of test_params.js but I can't figure how to register the xpctest_params component in a cpp unit test. I will attach it, though at this point this bug is blocking progression of my other bug, that is a regression, so I wonder if we may take the fix in the meanwhile I try to figure how to make a working test.

Comment on attachment 693304 [details] [diff] [review] patch v1.1 ehr attached the wrong version

attempt at test_params.js

Regarding the tests attachments: I'm not sure what to do with my conversion of TestXPTCInvoke to a cpp unit test, do you think it has any value? I'm fine with moving it to its own bug and get some sort of rs on landing it, provided it adds any testing value to the existing harness. The addition to test_params works, but is not needed for this specific bug, maybe I should just throw it away... TestParams.cpp is the attempt to make a cpp version of test_params.js, though AutoRegister can't find the components folder, or at least looks like that, I'm not that much into registration internals.

Comment on attachment 693312 [details] [diff] [review] attempt to a TestParams cpp unit test Review of attachment 693312 [details] [diff] [review]: ----------------------------------------------------------------- Note that C++ unit tests are now run: 1) With CWD as a temp dir 2) From the binary where they were built, not dist/bin See http://mxr.mozilla.org/mozilla-central/source/testing/runcppunittests.py for details. I intend to eventually get the C++ unit tests into the test package and run them on the test slaves, so it'd be good if we didn't add any more dependencies on the srcdir. ::: js/xpconnect/tests/cpp/TestParams.cpp @@ +110,5 @@ > + nsresult rv = NS_GetComponentRegistrar(getter_AddRefs(registrar)); > + do_check_true(NS_SUCCEEDED(rv), "Could get registrar"); > + if (registrar) { > + nsCOMPtr<nsIFile> file = do_get_file(NS_LITERAL_STRING("../../../../_tests/xpcshell/js/xpconnect/tests/components/js/xpctest.manifest")); > + test_component(NS_LITERAL_CSTRING("@mozilla.org/js/xpc/test/js/Params;1")); This is probably what's breaking you, $cwd is a temp dir so this path is never going to work. You should just do some extra file copying (using INSTALL_TARGETS: http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1532 ) to put the files you need in the same directory in the objdir as the C++ unit test binary.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20) > This is probably what's breaking you, $cwd is a temp dir so this path is > never going to work. I am actually not using cwd, but NS_OS_CURRENT_PROCESS_DIR, and the path was working (nsIFile.exists() returns true), though this is still not finding the components, maybe cpp unit tests just search for components implementations in the cwd folder? I will try to do some file copying of the components files when possible.

the fact the fix works is verified by this try run https://tbpl.mozilla.org/?tree=Try&rev=98e50713601e without the fix test_history_observer.js was perma-failing on Mac builds

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2087aaedc5 I'll keep working on the test as soon as I'm done with other priorities for the release, in the hope to get something able to verify the fix. flagging in-testsuite? for that.

IIRC I tried to complete the test, but didn't have any luck reproducing the faulty behavior in the test itself. That's a pity but at this point I'm not sure I even still have the patch around.