Closed Bug 784345 Opened 8 years ago Closed 8 years ago

Assertion failure: v.isObject(), at jsnum.cpp:1391


(Core :: JavaScript Engine, defect, critical)

Not set



Tracking Status
firefox14 --- unaffected
firefox15 --- unaffected
firefox16 --- unaffected
firefox17 + fixed
firefox18 + fixed
firefox-esr10 --- unaffected


(Reporter: decoder, Assigned: shu)


(Blocks 1 open bug)


(4 keywords, Whiteboard: [jsbugmon:update][adv-track-main17-])


(1 file, 5 obsolete files)

The following test asserts on mozilla-central revision f9a8fdb08193 (options -m -n -a):

function f(v, p) { return v*p; }
var p = new ParallelArray([,10]);
var s = p.scan(f);

Opt-builds don't crash on this but cause an infinite loop. Not sure if that is intended.
Attached patch fix and testcase (obsolete) — Splinter Review
My assumption about JS_ARRAY_HOLE being converted to undefined automatically by functions like ToUint32 was so completely wrong.
Attachment #653886 - Flags: review?(dmandelin)
Comment on attachment 653886 [details] [diff] [review]
fix and testcase

Cancelling rewrite pending patch rewrite. The way I deal with holes with PAs in general is incorrect.
Attachment #653886 - Flags: review?(dmandelin)
Attached patch fix and testcases (obsolete) — Splinter Review
Fix dealing with holes.

Some comments:

- Return a hole for out of bounds
- Holes in the underlying dense array store are passed through
- *All* operations except [] fill in undefined for holes. [] is the only operation that walks the prototype.
- |GetElementFromArrayLikeObject| now only fills in undefined for holes if the prototype of the array-like object does not have indexed properties
Attachment #653886 - Attachment is obsolete: true
Attachment #654002 - Flags: review?(dmandelin)
Comment on attachment 654002 [details] [diff] [review]
fix and testcases

Review of attachment 654002 [details] [diff] [review]:

Looks OK, but I'm sending it back for revision so you can fix the name and decide if the optional suggestions are worthwhile.

::: js/src/builtin/ParallelArray.cpp
@@ +65,5 @@
>      return obj;
>  }
> +static inline
> +void

Nit: static inline void on one line.

@@ +66,5 @@
>  }
> +static inline
> +void
> +FillHole(MutableHandleValue vp)

This name is too cutesy. I think of a hole as something within an array so the name suggests it is modifying an array. Something like 'ConvertHole' would be better.

@@ +158,2 @@
>              return true;
> +        }

It seems slightly nicer in this case to put one call to FillHole at the end, to make it less likely to miss adding one during maintenance.

@@ +1479,5 @@
> +            return false;
> +    } else {
> +        if (!obj->getParallelArrayElement(cx, iv, args.rval()))
> +            return false;
> +    }

I notice this kind of repeated branch seems to keep occurring. It seems preferable to factor it out if possible.
Attachment #654002 - Flags: review?(dmandelin) → review-
Attached patch holes rewrite and testcases (obsolete) — Splinter Review
Thank for the comments! The refactoring points are good ones.

The patch was mostly rewritten in favor of this comment:

Specific changes:
- Holes are filled in eagerly for the copying constructor form. The
  comprehension constructor and all operations produce dense arrays with no
  holes by construction.
- Added getParallelArrayElement(JSContext *, uint32_t, IndexInfo *,
  MutableHandleValue), which refactors the following pattern:

    if (source->isOneDimensional())
        source->getElementFromOnlyDimension(cx, i, &elem);
        source->getParallelArrayElement(cx, iv, &elem);

  The new method takes a pointer to an IndexInfo instead of a reference so
  that it's nullable.

Small cleanup:
- Some functions got shuffled around due to dependencies.
- Removed inOutermostDimensionRange() functions. Awkward name. Easier to just
  type i < obj->outermostDimension().
Attachment #654002 - Attachment is obsolete: true
Attached patch holes rewrite and testcases v2 (obsolete) — Splinter Review
Small nit fix
Attachment #654122 - Attachment is obsolete: true
Attached patch holes rewrite and testcases v3 (obsolete) — Splinter Review
Make the behavior that ParallelArray objects never inheriting indexed properties from the prototype more consistent. For an index i and a ParallelArray p,

 - |i in p| is true iff is an own property on p
 - When enumerating using |for (var prop in p)|, only p's own index properties are enumerated
Attachment #654125 - Attachment is obsolete: true
(In reply to Shu-yu Guo [:shu] from comment #5)
> Created attachment 654122 [details] [diff] [review]
> holes rewrite and testcases
> Thank for the comments! The refactoring points are good ones.
> The patch was mostly rewritten in favor of this comment:

Yeah, I figured--I jumped the gun and reviewed the earlier patch before properly digesting the comment you wrote below.
Attachment #654383 - Flags: review?(dvander)
Comment on attachment 654383 [details] [diff] [review]
holes rewrite and testcases v3

Flipping r? back to dmandelin
Attachment #654383 - Flags: review?(dvander) → review?(dmandelin)
Comment on attachment 654383 [details] [diff] [review]
holes rewrite and testcases v3

Review of attachment 654383 [details] [diff] [review]:

::: js/src/builtin/ParallelArray.h
@@ +158,5 @@
> +    // Otherwise, the IndexInfo parameter iv must be initialized and
> +    // iv->indices.length() must be 1. The value of iv->indices[0] is set to
> +    // index, and the general case above is called.
> +    bool getParallelArrayElement(JSContext *cx, uint32_t index, IndexInfo *maybeIV,
> +                                 MutableHandleValue vp);

Looks good. The only thing that looks a little funky is the interface to this function, although I see that its purpose is to allow various call sites to just call this one function (which is good) and that that's not so easy to do cleanly. IIUC, you definitely need an initialized IndexInfo in the multidimensional case, but you don't want to have to initialize one if you don't need to. If the latter actually isn't very important, then you could just make those call sites use the general case.

Otherwise, I'm now not entirely sure which way is better: the old way or the new way. In the old way, you need a conditional incantation to get the right element, which is a bit tricky. In the new way, there's just one function to call, which is nicer, but the function has some dependent preconditions that correspond to the conditional. I'm inclined to say that which way is better depends on what the call sites look like, so I'll just leave that to you.

I will suggest that this version of the function would be easier to understand with a comment like this:

// Get the element at |index| in the outermost dimension. This is a convenience
// function designed to require an IndexInfo only if it is actually needed.
// If the parallel array is multidimensional, then the caller must provide
// an IndexInfo initialized to length 1, which is used to access the array.
// This argument is modified. If the parallel array is one-dimensional, then
// |maybeIV| may be null.
Attachment #654383 - Flags: review?(dmandelin) → review+
Duplicate of this bug: 784342
Applied dmandelin's comment suggestion.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,verify-branch=mozilla-aurora]
Whiteboard: [jsbugmon:update,verify-branch=mozilla-aurora] → [jsbugmon:update,verify-branch=mozilla-aurora,ignore]
JSBugMon: This bug has been automatically confirmed to be still valid on branch mozilla-aurora  (reproduced on revision 02ba8b87cc48).
Tracking this for aurora and marking s-s because bugs that are dup to this one are s-s.
Whiteboard: [jsbugmon:update,verify-branch=mozilla-aurora,ignore] → [jsbugmon:update]
Duplicate of this bug: 786017
Assignee: general → shu
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
JSBugMon: This bug has been automatically verified fixed.
Attached patch patchSplinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 778559
User impact if declined: Crashes and heap memory reads
Testing completed (on m-c, etc.): locally and on m-c
Risk to taking this patch (and alternatives if risky): should be none. localized to an non-advertised (but enabled per contract with Intel) JavaScript API that isn't used in the wild yet.
String or UUID changes made by this patch: n/a

This is the patch as it appears in which has dmandelin's comments incorporated
Attachment #654383 - Attachment is obsolete: true
Attachment #656691 - Flags: approval-mozilla-aurora?
Attachment #656691 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-track-main17-]
Blocks: 778559
Group: core-security
Keywords: regression
You need to log in before you can comment on or make changes to this bug.