Closed Bug 1008236 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

Jan suggested this in bug 1007298.  He's totally right.
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.)
Attachment #8421241 - Flags: review?(bugs) → review+
> 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!
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
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.
Depends on: 1011810
Whiteboard: [need review]
https://hg.mozilla.org/mozilla-central/rev/8069054ebfc7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.