Closed Bug 898622 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: gkw, Assigned: nsm)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(4 files, 2 obsolete files)

Attached file 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.
new StructType(RegExp)

is another testcase for m-c rev 5aa02ee02f4b
Attached file stack for 2nd assert
new StructType(RegExp())

This similar testcase causes a different assert:

Assertion failure: IsPowerOfTwo(alignment), at jsutil.h
Summary: Assertion failure: isObjectOrNull(), at dist/include/js/Value.h → Assertion failure: isObjectOrNull(), at dist/include/js/Value.h or Assertion failure: IsPowerOfTwo(alignment), at jsutil.h
Attachment #781941 - Attachment description: stack → stack for 2nd assert
Attached file stack for crash
new StructType(Proxy.create(function() {}))

Crash [@ JS_EncodeString] with js::StructType::create on the stack.
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?
Crash Signature: [@ JS_EncodeString] [@ js::StructType::create]
Flags: needinfo?(nsm.nikhil)
Keywords: crash
Summary: Assertion failure: isObjectOrNull(), at dist/include/js/Value.h or Assertion failure: IsPowerOfTwo(alignment), at jsutil.h → 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
Whiteboard: [fuzzblocker][jsbugmon:update,bisect] → [fuzzblocker][jsbugmon:update]
Attachment #781969 - Attachment description: stack → stack for crash
See Also: → 898644
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.
Flags: needinfo?(nsm.nikhil)
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.
Crash Signature: [@ JS_EncodeString] [@ js::StructType::create] → [@ JS_EncodeString] [@ js::StructType::create]
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker] [jsbugmon:update]
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.
Crash Signature: [@ JS_EncodeString] [@ js::StructType::create] → [@ JS_EncodeString(JSContext*, JSString*) ] [@ js::StructType::create]
Attached patch Fix (obsolete) — Splinter Review
This fixes the StructType layout() and toString() failures
Attachment #782289 - Flags: review?(nmatsakis)
Assignee: general → nsm.nikhil
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.
Attachment #782289 - Attachment description: Fix StructType problems. → Fix
Blocks: 898695
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.
Attachment #782289 - Flags: review?(nmatsakis) → review+
Crash Signature: [@ JS_EncodeString(JSContext*, JSString*) ] [@ js::StructType::create] → [@ JS_EncodeString(JSContext*, JSString*) ] [@ js::StructType::create]
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision b8c7acba4b40).
Marking as leave open since this will need retesting once 898661 is undone.
Crash Signature: [@ JS_EncodeString(JSContext*, JSString*) ] [@ js::StructType::create] → [@ JS_EncodeString(JSContext*, JSString*) ] [@ js::StructType::create]
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update,ignore][leave open]
Attached patch Fix v2 (obsolete) — Splinter Review
Moves errors to StructType::layout(), and changes suggested in previous review.
Attachment #782289 - Attachment is obsolete: true
Attachment #783973 - Flags: review?(nmatsakis)
Attachment #783973 - Attachment is obsolete: true
Attachment #783973 - Flags: review?(nmatsakis)
Remove fallback error from layout() tail.
Attachment #784040 - Flags: review?(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
Attachment #784040 - Flags: review?(nmatsakis) → review+
Bug 898347 enabled Binary Data a while ago. Marking as fixed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore][leave open]
JSBugMon can verify this.
Whiteboard: [jsbugmon:update]
Depends on: 898347
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.