Closed Bug 866730 Opened 7 years ago Closed 7 years ago

Assertion failure: lastProperty()->hasSlot() && getSlot(lastProperty()->slot()).isUndefined(), at vm/Shape.cpp:931

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: decoder, Unassigned)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

The following testcase asserts on mozilla-central revision 05533d50f2f7 (no options required):


new Unicode(1328);
function Unicode(c) {
  this.lower = this.toString + this[this.substring = "foo"]--;
}
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   129598:d746d516bf55
user:        Brian Hackett
date:        Mon Apr 15 17:12:51 2013 -0600
summary:     Bug 862103 - Various benchmark performance fixes.

This iteration took 136.938 seconds to run.
Ccing bhackett based on comment 1 :)
Attached patch patchSplinter Review
The definite properties for this script weren't being computed properly.  This seems to be because AnalyzePoppedThis will visit all 'this' values on the pending stack even if their offset hasn't been seen yet.  The attached patch structures things so that popped 'this' values are only processed after the offset at which they are popped is visited, allowing a lot of streamlining in this area in the process.  Most of this patch is just adjusting the indenting in AnalyzePoppedThis.
Attachment #744224 - Flags: review?(shu)
Comment on attachment 744224 [details] [diff] [review]
patch

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

Nice refactor.

::: js/src/jsinfer.cpp
@@ +5132,5 @@
>           * End analysis after the first return statement from the script,
>           * returning success if the return is unconditional.
>           */
> +        if (op == JSOP_RETURN || op == JSOP_STOP || op == JSOP_RETRVAL)
> +            return code->unconditional;

Do we need to check if there are pending 'this' uses and NULL out |state.baseobj| here? |CheckNewScriptProperties| checks |!state.baseobj|.

@@ +5137,4 @@
>  
>          /* 'this' can escape through a call to eval. */
> +        if (op == JSOP_EVAL)
> +            return false;

Ditto.

@@ -5205,5 @@
> -    if (entirelyAnalyzed &&
> -        !pendingPoppedThis.empty() &&
> -        !AnalyzePoppedThis(cx, &pendingPoppedThis, type, fun, state))
> -    {
> -        return false;

Oh good, we can remove this chunk.

@@ +5204,5 @@
> +
> +        unsigned slotSpan = state.baseobj->slotSpan();
> +        RootedValue value(cx, UndefinedValue());
> +        if (!DefineNativeProperty(cx, state.baseobj, id, value, NULL, NULL,
> +                                  JSPROP_ENUMERATE, 0, 0, DNP_SKIP_TYPE)) {

Nit: I guess while you're reindenting this code, move { to the newline.

@@ +5313,5 @@
> +            RawObject scriptObj = scriptTypes->getSingleton();
> +            if (!funcallObj || !funcallObj->isFunction() ||
> +                funcallObj->toFunction()->isInterpreted() ||
> +                !scriptObj || !scriptObj->isFunction() ||
> +                !scriptObj->toFunction()->isInterpreted()) {

Nit: { to new line.
Attachment #744224 - Flags: review?(shu) → review+
Clearing |state.baseobj| in the cases you mentioned isn't necessary.  Any pending |this| values are just points where |this| was pushed but hasn't been popped yet, and are conceptually no different from |this| values pushed after analysis stopped on the script in question.  Ideally we should never have to clear |state.baseobj| when analyzing new 'this' properties, and rather just bail out at some point leaving prior information intact.  The points where we do clear |state.baseobj| are effectively internal analysis errors like hitting OOM or putting the object into dictionary mode.

https://hg.mozilla.org/integration/mozilla-inbound/rev/05098ee6e6c8
https://hg.mozilla.org/mozilla-central/rev/05098ee6e6c8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.