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

VERIFIED FIXED in Firefox 17

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: shu)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla18
x86_64
Linux
assertion, regression, testcase, verifyme
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 unaffected, firefox15 unaffected, firefox16 unaffected, firefox17+ fixed, firefox18+ fixed, firefox-esr10 unaffected)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 653886 [details] [diff] [review]
fix and testcase

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

Updated

6 years ago
Attachment #653886 - Flags: review?(dmandelin)
(Assignee)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
Created attachment 654002 [details] [diff] [review]
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
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-
(Assignee)

Comment 5

6 years ago
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

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().
Attachment #654002 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 654125 [details] [diff] [review]
holes rewrite and testcases v2

Small nit fix
Attachment #654122 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
Created attachment 654383 [details] [diff] [review]
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
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:
> 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.
(Assignee)

Updated

6 years ago
Attachment #654383 - Flags: review?(dvander)
(Assignee)

Comment 9

6 years ago
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+
(Assignee)

Updated

6 years ago
Duplicate of this bug: 784342
(Assignee)

Comment 12

6 years ago
Applied dmandelin's comment suggestion.

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

Updated

6 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,verify-branch=mozilla-aurora]
(Reporter)

Updated

6 years ago
Whiteboard: [jsbugmon:update,verify-branch=mozilla-aurora] → [jsbugmon:update,verify-branch=mozilla-aurora,ignore]
(Reporter)

Comment 13

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

Comment 14

6 years ago
Tracking this for aurora and marking s-s because bugs that are dup to this one are s-s.
Group: core-security
status-firefox-esr10: --- → unaffected
status-firefox14: --- → unaffected
status-firefox15: --- → unaffected
status-firefox16: --- → unaffected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
(Reporter)

Updated

6 years ago
Whiteboard: [jsbugmon:update,verify-branch=mozilla-aurora,ignore] → [jsbugmon:update]
(Assignee)

Updated

6 years ago
Duplicate of this bug: 786017
tracking-firefox17: ? → +
tracking-firefox18: ? → +
https://hg.mozilla.org/mozilla-central/rev/91d39d72ac86
Assignee: general → shu
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox18: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 17

6 years ago
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Comment 18

6 years ago
Created attachment 656691 [details] [diff] [review]
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
Attachment #654383 - Attachment is obsolete: true
Attachment #656691 - Flags: approval-mozilla-aurora?
Attachment #656691 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
status-firefox17: affected → fixed
Keywords: verifyme
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.