1651210 - uint scalars are treated as 32-bit signed integers

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Mark Banner (:standard8)]

I noticed this as I wanted to be able to store an nsresult (uint32) value into a scalar. These values are typically more than the maximum value for the signed integer.

When we store these values the are coming back as if they were signed integers.

STR:

1. Change the expectedUint value in the test_TelemetryScalars.js to be 2147549183:
2. Run the test.

Expected Results => Test should pass.

Actual Results => Test fails:

  FAIL test_serializationFormat - [test_serializationFormat : 48] telemetry.test.unsigned_int_kind must have the correct value. - -2147418113 == 2147549183
/Users/mark/dev/gecko/objdir-ff-opt/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js:test_serializationFormat:48

Comment 1

[Chris H-C :chutten]

Documenting my initial investigation: We store it as a uint32_t, so it can't possibly be negative in C++ storage in ScalarUnsigned.

To snapshot it out to storage we stuff it into an nsIVariant using SetUint32 then turn that variant into a JS Value using nsContentUtils::XPConnect()->VariantToJS. Maybe we're misusing those APIs somehow?

Comment 2

[Mark Banner (:standard8)]

I just noticed a warning:

browser.searchinit.init_result_status_code - Truncating float/double number.

That leads to: https://searchfox.org/mozilla-central/rev/7ec7ee4a9bde171ba195ab46ed6077e4baaef34d/toolkit/components/telemetry/core/TelemetryScalar.cpp#614

If I understand that code right, Javascript is probably thinking we've sent the API a double value rather than an unsigned int.

Comment 3

[Chris H-C :chutten]

That warning might be a false lead. Attaching a debugger, we still get the correct 2147549183 value in ScalarUnsigned::SetValue. gdb at least seems pretty confident that the data is getting into C++ correctly.

Here's the flow according to my debugging:

1. Telemetry.scalarSet("telemetry.test.unsigned_int_kind", 2147549183)
2. Big uint value gets treated as a double inside the JSVal (ie, isInt32() is false isDouble() is true) (perhaps because it's too big for an int32, and JSVal doesn't have a Uint32 type)
3. Double in JSVal is propagated as a Double in nsIVariant using JSToVariant (ie, GetDataType returns 9 (nsIDataType::VTYPE_DOUBLE))
4. Warning is printed, but Telemetry still correctly gets the uint32_t value (2147549183) out.
5. Telemetry.getSnapshotForScalars("main")
6. At some point ScalarUnsigned::GetValue is called to create a nsIVariant which is a Uint32 (ie, GetDataType returns 6 (nsIDataType::VTYPE_UINT32))
7. Then VariantToJS puts it into a Int32 JSVal (ie, scalarJsValue.isInt32() returns true)
8. It's then serialized into an object structure and returned to the caller.

So the problem is Step 6. I'm not sure how, but the nsIVariant which knows it is uint32 and has the value 2147549183 is being turned into a JSVal which is an int32. Near as I can tell from tracing it's supposed to check if the value is int32 before setting it, and will set it as a double (and make isDouble() return true) if it doesn't fit.

More investigation needed.

Comment 4

[Chris H-C :chutten]

I've tracked it to nsDiscriminatedUnion which has mType of VTYPE_UINT32 (6) and the correct value (u.mUint32Value is 2147549183), but when ConvertToDouble runs it pulls the int32 value instead and casts it to double.

gdb's not unrolling the macros so I'm not exactly sure why it's doing this. From what I can read of the source it should jump to the VTYPE_UINT32 case and use the u.mUint32Value, casting to double.

From blame, I should be asking questions of the :froydnj and :mccr8 of 2015. Since they're unavailable, I'll needinfo their current incarnations : )

To sum: I have a XPCVariant with a value set using SetAsUint32 that I'm calling VariantToJS on and am getting a negative value out. This appears to be because the code is pulling the int32 value out of the discriminated union in ConvertToDouble but I can't figure out how or why.

Comment 5

[Andrew McCreight [:mccr8]]

I'm not sure what is going wrong, but I noticed there's a bug on this line AFAICT:
https://searchfox.org/mozilla-central/rev/85ae3b911d5fcabd38ef315725df32e25edef83b/xpcom/ds/nsVariant.cpp#81

In the uint32 case, it is doing this: aOutData->u.mInt32Value = u.mUint32Value;. Shouldn't it be u.mUint32Value?

Comment 6

[Andrew McCreight [:mccr8]]

The next line also sets the type to int32 and not uint32...

Comment 7

[Chris H-C :chutten]

I don't see it diving into ToManageableNumber, though. gdb is convinced it jumps into this NUMERIC_CONVERSION_METHOD_* block which does its own return value conversion (differently from toManageableNumber).

Comment 8

[Nathan Froyd [:froydnj]]

mccr8's bug correlates a lot better with the initial report, though. It would also explain:

(In reply to Chris H-C :chutten from comment #4)

I've tracked it to nsDiscriminatedUnion which has mType of VTYPE_UINT32 (6) and the correct value (u.mUint32Value is 2147549183), but when ConvertToDouble runs it pulls the int32 value instead and casts it to double.

I don't know that the mInt32Value vs. mUint32Value matters a whole lot, because those members should occupy the same space in the union. But:

https://searchfox.org/mozilla-central/rev/85ae3b911d5fcabd38ef315725df32e25edef83b/xpcom/ds/nsVariant.cpp#81-82

getting reinterpreted as a signed integer seems like a significant problem.

Comment 9

[Chris H-C :chutten]

Well I'll be hornswoggled, because fixing ToManageableNumber does indeed fix this bug. Patch incoming.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Chris H-C :chutten from comment #9)

Well I'll be hornswoggled, because fixing ToManageableNumber does indeed fix this bug. Patch incoming.

I suspect that the debug information generated by the compiler (and being interpreted by GDB) was leading you astray...unless you were compiling without any optimization at all, at which point I also am hornswoggled.

Comment 11

[Andrew McCreight [:mccr8]]

This bug appears to date to the initial landing of the variant code in 2001: https://searchfox.org/mozilla-central/diff/900a5bfba9a236b96493a7b250e6ea67a9febfd2/xpcom/ds/nsVariant.cpp#87

Comment 12

[Chris H-C :chutten]

Attachment: Bug 1651210 - uint variant values should convert over as uints, not ints r?froydnj!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7130ca82d6cd
uint variant values should convert over as uints, not ints r=froydnj

Comment 14

[Chris H-C :chutten]

(In reply to Nathan Froyd [:froydnj] from comment #10)

(In reply to Chris H-C :chutten from comment #9)

Well I'll be hornswoggled, because fixing ToManageableNumber does indeed fix this bug. Patch incoming.

I suspect that the debug information generated by the compiler (and being interpreted by GDB) was leading you astray...unless you were compiling without any optimization at all, at which point I also am hornswoggled.

I tried my best with my .mozconfig having both --enable-debug and --disable-optimize so... ¯\_(ツ)_/¯

Comment 15

[Nathan Froyd [:froydnj]]

Note that nsDiscriminatedUnion::ConvertToDouble:

https://searchfox.org/mozilla-central/rev/8d55e18875b89cdf2a22a7cba60dc40999c18356/xpcom/ds/nsVariant.cpp#528-532

will wind up calling into ToManageableNumber:

https://searchfox.org/mozilla-central/rev/8d55e18875b89cdf2a22a7cba60dc40999c18356/xpcom/ds/nsVariant.cpp#418-422

so that might explain things.

Comment 16

[Chris H-C :chutten]

Gah, I skipped the preamble and jumped straight to the fromuint case when I saw the switch. That's the missing piece, thank you!

Comment 17

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/7130ca82d6cd