898622 - Crash [@ JS_EncodeString] or [@ js::StructType::create] or Assertion failure: isObjectOrNull(), at dist/include/js/Value.h or Assertion failure: IsPowerOfTwo(alignment), at jsutil.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Array.prototype.filter.call([0], StructType.prototype.toString)

asserts js (non-deterministic non-threadsafe) debug shell on m-c changeset 5aa02ee02f4b without any CLI arguments at Assertion failure: isObjectOrNull(), at dist/include/js/Value.h

Setting fuzzblocker because this is happening very frequently.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

new StructType(RegExp)

is another testcase for m-c rev 5aa02ee02f4b

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for 2nd assert

new StructType(RegExp())

This similar testcase causes a different assert:

Assertion failure: IsPowerOfTwo(alignment), at jsutil.h

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for crash

new StructType(Proxy.create(function() {}))

Crash [@ JS_EncodeString] with js::StructType::create on the stack.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/43d1eada77d6
user:        Nikhil Marathe
date:        Thu Jul 25 17:59:47 2013 -0700
summary:     Bug 578700 - StructType and BinaryStruct implementation. r=nmatsakis

Nikhil, is bug 578700 a possible regressor?

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Binary Data is disabled in release builds, so it shouldn't cause any security issues or regressions in the wild, and we (me and nmatsakis) are hoping to quickly fix the common issues. Would it make sense to temporarily switch off BD even on nightly? I'd hate to actually backout the changesets since the patches have been around on Bugzilla for a *long* time and bitrotted several times already.

Comment 6

[Jeff Walden [:Waldo]]

Can you just leave in #ifdefs around the places that add the property to the global object (enumeration, resolve)?  We do that now for Intl stuff, most of which builds (with some stubbing), but which is not quite enabled in desktop builds yet.

Comment 7

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/43d1eada77d6
user:        Nikhil Marathe
date:        Thu Jul 25 17:59:47 2013 -0700
summary:     Bug 578700 - StructType and BinaryStruct implementation. r=nmatsakis

This iteration took 349.205 seconds to run.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix

This fixes the StructType layout() and toString() failures

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 782289 [details] [diff] [review]
Fix

bzexport refreshed the whole patch.
Also adds:
Guard toObjectOrNull() with isObject() checks.
Switch to InformalValueTypeName() to reduce code and allocations.

Comment 10

[Niko Matsakis [:nmatsakis]]

Comment on attachment 782289 [details] [diff] [review]
Fix

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

::: js/src/builtin/BinaryData.cpp
@@ +1510,5 @@
>      AutoIdVector fieldProps(cx);
>      if (!GetPropertyNames(cx, fields, JSITER_OWNONLY, &fieldProps))
>          return false;
>  
> +    if (fieldProps.length() <= 0)

Should we report an error here? In particular, we should clarify whether an empty struct type is legal.

@@ +1729,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    if (!args.thisv().isObject())
> +        return false;

Again, I think we should report an error.

@@ +1735,4 @@
>      RootedObject thisObj(cx, args.thisv().toObjectOrNull());
>  
>      if (!IsStructType(thisObj))
>          return false;

Again, I think we should report an error.

Comment 11

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision b8c7acba4b40).

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Marking as leave open since this will need retesting once 898661 is undone.

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix v2

Moves errors to StructType::layout(), and changes suggested in previous review.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix StructType problems.

Remove fallback error from layout() tail.

Comment 15

[Niko Matsakis [:nmatsakis]]

Comment on attachment 784040 [details] [diff] [review]
Fix StructType problems.

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

::: js/src/builtin/BinaryData.cpp
@@ +1509,5 @@
>  {
>      AutoIdVector fieldProps(cx);
> +    if (!GetPropertyNames(cx, fields, JSITER_OWNONLY, &fieldProps)) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                             JSMSG_NOT_ITERABLE, structType->getClass()->name);

You should not report an error here...GetPropertyNames ought to report the error itself, as it takes a JSContext* argument.

@@ +1530,5 @@
>  
>      for (unsigned int i = 0; i < fieldProps.length(); i++) {
>          RootedValue fieldTypeVal(cx);
>          RootedId id(cx, fieldProps[i]);
> +        if (!JSObject::getGeneric(cx, fields, fields, id, &fieldTypeVal)) {

nit: no braces needed

@@ +1581,5 @@
>      if (!JS_DefineProperty(cx, structType, "bytes",
>                             Int32Value(structByteSize), NULL, NULL,
> +                           JSPROP_READONLY | JSPROP_PERMANENT)) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                             JSMSG_ACCESSOR_DEF_DENIED, structType->getClass()->name);

Not your job to report an error here, JS_DefineProperty ought to do it.

Nit: braces (will be) unnecessary

@@ +1672,5 @@
>          return NULL;
>      }
>  
>      RootedObject fieldsProto(cx);
> +    if (!JSObject::getProto(cx, fields, &fieldsProto)) {

Nit: braces unnecessary

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b6b697ead534

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Bug 898347 enabled Binary Data a while ago. Marking as fixed.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

JSBugMon can verify this.