1008236 - Assert in the non-Ion codepaths that binding return value types match the JitInfo return type

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Jan suggested this in bug 1007298.  He's totally right.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Assert that binding generic getters/setters/methods return values that match the return type claimed in the jitinfo

Comment 2

[Boris Zbarsky [:bzbarsky]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=5dcb333d8b50

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8421241 [details] [diff] [review]
Assert that binding generic getters/setters/methods return values that match the return type claimed in the jitinfo

>+#ifdef DEBUG
>+void
>+AssertReturnTypeMatchesJitinfo(const JSJitInfo* aJitInfo,
>+                               JS::Handle<JS::Value> aValue)
>+{
>+  switch (aJitInfo->returnType()) {
>+  case JSVAL_TYPE_UNKNOWN:
>+    // Any value is good.
>+    break;
>+  case JSVAL_TYPE_DOUBLE:
>+    // The value could actually be an int32 value as well.
>+    MOZ_ASSERT(aValue.isNumber());
>+    break;
>+  case JSVAL_TYPE_INT32:
>+    MOZ_ASSERT(aValue.isInt32());
>+    break;
>+  case JSVAL_TYPE_UNDEFINED:
>+    MOZ_ASSERT(aValue.isUndefined());
>+    break;
>+  case JSVAL_TYPE_BOOLEAN:
>+    MOZ_ASSERT(aValue.isBoolean());
>+    break;
>+  case JSVAL_TYPE_STRING:
>+    MOZ_ASSERT(aValue.isString());
>+    break;
>+  case JSVAL_TYPE_NULL:
>+    MOZ_ASSERT(aValue.isNull());
>+    break;
>+  case JSVAL_TYPE_OBJECT:
>+    MOZ_ASSERT(aValue.isObject());
>+    break;
>+  default:
>+    // Someone messed up their jitinfo type.
>+    MOZ_ASSERT(false, "Unexpected JSValueType stored in jitinfo");
>+    break;
>+  }
>+}
I assume we don't have any way to assert here that we've handled all the possible cases.
Though, I guess new types aren't added to js too often.


(I don't know what JSVAL_TYPE_MAGIC is, and hopefully bindings don't need to care about it.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I assume we don't have any way to assert here that we've handled all the possible cases.

You mean assert at compile time?

At runtime, we do assert that we know how to check sanity here for the type our jitinfo is claiming...

JSVAL_TYPE_MAGIC is internal JIT stuff that we don't have to worry about, yes.

Thanks for the review!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out because something in the push was causing mochitest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b99b42f1337

https://tbpl.mozilla.org/php/getParsedLog.php?id=39747566&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39747507&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39747838&tree=Mozilla-Inbound

Comment 6

[Ryan VanderMeulen [:RyanVM]]

This push caused a large number of B2G debug mochitest failures that contributed to a large bustage pileup on inbound and led to having to revert to a last-good cset, inconveniencing other innocent developers as well. These same failures occurred with yesterday's landing. Please give this a full Try run before attempting to re-land.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2

https://tbpl.mozilla.org/php/getParsedLog.php?id=39821101&tree=Mozilla-Inbound

Comment 7

[Boris Zbarsky [:bzbarsky]]

Looks like the ICC code has an "nsISupports" return value where they then go and return null.  This doesn't crash at runtime (though we should fix that)... at least until the code gets jitted.

I filed bug 1011810 on that.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: For posterity, the patch I used to debug this

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fwiw, with bug 1011810 fixed: https://tbpl.mozilla.org/?tree=Try&rev=aa4c1be892bf

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8069054ebfc7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8069054ebfc7