Closed
Bug 821628
Opened 12 years ago
Closed 12 years ago
xptcstubs_x86_64_darwin is wrongly converting some bool params
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: mak, Assigned: mak)
References
(Depends on 1 open bug)
Details
Attachments
(4 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
checkpointing the patch, now looking into the tests.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #692283 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
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
Assignee | ||
Comment 7•12 years ago
|
||
Found a bug in the test. Unfortunately now the test passes, so it's not good for my purpose.
Attachment #692302 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
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).
Flags: needinfo?(benjamin)
Assignee | ||
Comment 9•12 years ago
|
||
I guess I should look into bug 684327
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(benjamin)
Assignee | ||
Comment 10•12 years ago
|
||
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
Comment 11•12 years ago
|
||
I'm not sure what information you need... TextXPTCInvoke has been dead for years, as far as I know.
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
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.
Attachment #692229 -
Attachment is obsolete: true
Attachment #693304 -
Flags: review?(benjamin)
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 693304 [details] [diff] [review]
patch v1.1
ehr attached the wrong version
Attachment #693304 -
Attachment is obsolete: true
Attachment #693304 -
Flags: review?(benjamin)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #693307 -
Flags: review?(benjamin)
Assignee | ||
Updated•12 years ago
|
Attachment #692331 -
Attachment description: test v1.2 → convert TestXPTCInvoke to a cpp unit test v1.2
Assignee | ||
Comment 17•12 years ago
|
||
attempt at test_params.js
Assignee | ||
Comment 18•12 years ago
|
||
Assignee | ||
Comment 19•12 years ago
|
||
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 20•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
(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.
Assignee | ||
Comment 22•12 years ago
|
||
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
Updated•12 years ago
|
Attachment #693307 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 23•12 years ago
|
||
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.
Flags: in-testsuite?
Comment 24•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 25•9 years ago
|
||
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.
Flags: in-testsuite?
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•