838613 - we are getting a value of 0 for a pointer in nsExceptionHandler.cpp::RegisterAppMemory()

Status: NEW

(Core :: XPConnect, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

running xpcshell tests on ubuntu32 VMs yields an error in toolkit/crashreporter/test/unit/test_crashreporter_appmem.js.  This is the same build and tests as we run on fedora 32 unittests which happen to pass.

We run the test just fine on Ubuntu 64 bit, so the 32 bit is the only problem.  After talking to :ted about this, I added some printf lines to the crashreporter:
diff -r 61a7b7d2a090 toolkit/crashreporter/nsExceptionHandler.cpp
--- a/toolkit/crashreporter/nsExceptionHandler.cpp	Tue Feb 05 16:14:58 2013 -0500
+++ b/toolkit/crashreporter/nsExceptionHandler.cpp	Wed Feb 06 10:07:09 2013 -0500
@@ -87,6 +87,9 @@
 #include "mozilla/mozalloc_oom.h"
 #include "mozilla/mozPoisonWrite.h"
 
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
+
 #if defined(XP_MACOSX)
 CFStringRef reporterClientAppID = CFSTR("org.mozilla.crashreporter");
 #endif
@@ -1531,6 +1534,7 @@
 
 nsresult RegisterAppMemory(void* ptr, size_t length)
 {
+  printf_stderr("RegisterAppMemory: ptr = %" PRIu64 "\n", (uint64_t)ptr);
   if (!GetEnabled())
     return NS_ERROR_NOT_INITIALIZED;
 
diff -r 61a7b7d2a090 toolkit/xre/nsAppRunner.cpp
--- a/toolkit/xre/nsAppRunner.cpp	Tue Feb 05 16:14:58 2013 -0500
+++ b/toolkit/xre/nsAppRunner.cpp	Wed Feb 06 10:07:09 2013 -0500
@@ -96,6 +96,9 @@
 
 #include "mozilla/unused.h"
 
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
+
 using namespace mozilla;
 using mozilla::unused;
 using mozilla::scache::StartupCache;
@@ -1013,6 +1016,7 @@
 nsXULAppInfo::RegisterAppMemory(uint64_t pointer,
                                 uint64_t len)
 {
+  printf_stderr("RegisterAppMemory, pointer = %" PRIu64 "\n", pointer);
   return CrashReporter::RegisterAppMemory((void *)pointer, len);
 }
 

Then I ran the test and got this output:
cltbld@tst-jmaher-ubuntu32-001:~/test/xpcshell$ python runxpcshelltests.py --manifest tests/all-test-dirs.list --test-path=toolkit/crashreporter/test/unit/test_crashreporter_appmem.js ../firefox/xpcshell 
TEST-INFO | profile dir is /tmp/xpcshell/xpcshellprofile
TEST-INFO | /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>
RegisterAppMemory: ptr = 18446744072491674912
RegisterAppMemory: ptr = 18446744072491707776
RegisterAppMemory: ptr = 18446744072491740640

TEST-INFO | (xpcshell/head.js) | test 1 pending
RegisterAppMemory: ptr = 18446744072492006688
RegisterAppMemory: ptr = 18446744072492039552
RegisterAppMemory: ptr = 18446744072492072416
RegisterAppMemory, pointer = 18446744069414584320
RegisterAppMemory: ptr = 0

TEST-PASS | /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/head_crashreporter.js | [do_crash : 65] 256 != 0

TEST-PASS | /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/head_crashreporter.js | [handleMinidump : 97] true == true

TEST-PASS | /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js | [null : 8] true == true

TEST-PASS | /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js | [null : 9] true == true
2013-02-06 06:46:58: minidump.cc:3833: INFO: Minidump opened minidump /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/635d71f7-11d2-b45a-08e8d892-48c74b24.dmp
2013-02-06 06:46:58: minidump.cc:3944: INFO: Minidump not byte-swapping minidump
2013-02-06 06:46:58: minidump.cc:2773: INFO: MinidumpMemoryList has no memory region at 0xffffffffb43155e4
2013-02-06 06:46:58: minidump.cc:3805: INFO: Minidump closing minidump

TEST-UNEXPECTED-FAIL | /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js | false == true - See following stack:
JS frame :: /home/cltbld/test/xpcshell/head.js :: do_throw :: line 461
JS frame :: /home/cltbld/test/xpcshell/head.js :: do_report_result :: line 563
JS frame :: /home/cltbld/test/xpcshell/head.js :: _do_check_eq :: line 573
JS frame :: /home/cltbld/test/xpcshell/head.js :: do_check_eq :: line 580
JS frame :: /home/cltbld/test/xpcshell/head.js :: do_check_true :: line 594
JS frame :: /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js :: <TOP_LEVEL> :: line 10
JS frame :: /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/head_crashreporter.js :: handleMinidump :: line 101
JS frame :: /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/head_crashreporter.js :: do_crash :: line 68
JS frame :: /home/cltbld/test/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js :: run_test :: line 7
JS frame :: /home/cltbld/test/xpcshell/head.js :: _execute_test :: line 325
JS frame :: -e :: <TOP_LEVEL> :: line 1
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | (xpcshell/head.js) | exiting test
<<<<<<<
INFO | Result summary:
INFO | Passed: 0
INFO | Failed: 1
INFO | Todo: 0


Ted suspect we are doing something where we get into a casting problem between JS and C.  For example he sees this:
07:22 <@ted> >>> hex(18446744069414584320)
07:22 <@ted> '0xffffffff00000000L'

Maybe this is a different kernel or some side effect of running inside a VM.

Comment 1

[(not currently active) Ted Mielczarek]

So the test is doing the following:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js#4

appAddr = CrashTestUtils.saveAppMemory();

which is:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/CrashTestUtils.jsm#37
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/nsTestCrasher.cpp#95
which returns a uint64_t via js-ctypes. 64-bit ints are a special type in js-ctypes for obvious reasons. It then does:
crashReporter.registerAppMemory(appAddr, 32);

passing that uint64_t to an xpidl method that takes "unsigned long long":
http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsICrashReporter.idl#83
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1013

which casts it to a void* and passes it through. It looks like the value is bogus by the time it gets to RegisterAppMemory, so perhaps the js-ctypes uint64_t is getting mangled by XPConnect? I'm not sure why this test would pass on 32-bit Fedora though.

Comment 2

[Bobby Holley (:bholley)]

It looks like XPCConvert::JSData2Native ends up invoking ValueToPrimitive, which is a templated function in dom/bindings, which should call JS::ToUint64. Does that do anything smart with a ctypes uint64_t? I kind of doubt it.

Comment 3

[(not currently active) Ted Mielczarek]

This works most of the time, so there must be *something*.

Comment 4

[Benjamin Smedberg]

The IDL specifies "in unsigned long long ptr".

If I'm reading XPCConvert correctly, this goes through ValueToPrimitive with a uint64_t*.  This ends up in JS::ToUint64, which has two paths, a fast path if v.isInt32() which should never be the case here, and a slow path js::ToUint64Slow which goes through js::ToNumberSlow which does an intermediate conversion to `double` through JSObject::defaultValue, which converts the ctypes object to a *string* via toString (because ctypes uint64 objects don't have a .toPrimitive method).

I tend to think that perhaps js::ToUint64Slow should have a special case for ctypes objects, but perhaps there are other reasonable options here also.

Comment 5

[Benjamin Smedberg]

ted, this is probably a case where the number fit into a double, so we didn't see the issue. If it doesn't fit into a double, it will be rounded to double precision and things can get wonky.

Although that doesn't quite explain the symptom of 0xffffffff00000000

Comment 6

[(not currently active) Ted Mielczarek]

Okay, so we believe that the xpconnect code here is actually broken for this case?

Comment 7

[Benjamin Smedberg]

Yeah, basically xpconnect has never dealt with real 64-bit integers, and so the special ctypes form of these ints are also broken.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: disable test on 32 bit linux only (1.0)

disable this test until we resolve the uint64 data handling

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 710846 [details] [diff] [review]
disable test on 32 bit linux only (1.0)

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

Please add a comment referencing this bug (and use [leave open] in the whiteboard).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03232247aef8