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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: fix and testcase

My assumption about JS_ARRAY_HOLE being converted to undefined automatically by functions like ToUint32 was so completely wrong.

Comment 2

[Shu-yu Guo [:shu]]

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.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: fix and testcases

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

Comment 4

[David Mandelin [: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.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: holes rewrite and testcases

Thank for the comments! The refactoring points are good ones.

The patch was mostly rewritten in favor of this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=784342#c5

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);
    else
        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().

Comment 6

[Shu-yu Guo [:shu]]

Attachment: holes rewrite and testcases v2

Small nit fix

Comment 7

[Shu-yu Guo [:shu]]

Attachment: holes rewrite and testcases v3

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

Comment 8

[David Mandelin [:dmandelin]]

(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:
> https://bugzilla.mozilla.org/show_bug.cgi?id=784342#c5

Yeah, I figured--I jumped the gun and reviewed the earlier patch before properly digesting the comment you wrote below.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 654383 [details] [diff] [review]
holes rewrite and testcases v3

Flipping r? back to dmandelin

Comment 10

[David Mandelin [: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.

Comment 12

[Shu-yu Guo [:shu]]

Applied dmandelin's comment suggestion.

https://hg.mozilla.org/integration/mozilla-inbound/rev/91d39d72ac86

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically confirmed to be still valid on branch mozilla-aurora  (reproduced on revision 02ba8b87cc48).

Comment 14

[Christian Holler (:decoder)]

Tracking this for aurora and marking s-s because bugs that are dup to this one are s-s.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91d39d72ac86

Comment 17

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: patch

[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 https://hg.mozilla.org/mozilla-central/rev/91d39d72ac86 which has dmandelin's comments incorporated

Comment 19

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99828bd1a3e9