514454 - Fix all the bugs introduced by the patch in bug 514222

Status: RESOLVED DUPLICATE of bug 505523

(Core :: JavaScript Engine, defect, P2)

Description

[Jason Orendorff [:jorendorff]]

> This [...] suppresses an error that we reasoned "can't happen"
> (for some IRC value of "reasoned" -- more permanent proofoid wanted).

I sat down to write the proof, and sure enough, the reasoning is buggy. We use getEmptyScopeShape in two places: slowifying an array, and clearing a scope. I introduced bugs in both cases.

1. The first time we slowify an array other than Array.prototype, getEmptyScope actually gets called and could fail with OOM.

2. When we clear a scope, the object must have been empty once, so its proto must already have an emptyScope, right? But we may have assigned to obj.__proto__, in which case the proto now is a different object which might not have an emptyScope. Then we would call getEmptyScope which could fail. In this case OWN_SHAPE is set, and we probably shouldn't be resetting the shape anyway.

Furthermore, if the reasoning *had* been correct, then all the code in getEmptyScopeShape is pointless and it should just say:

  /* Caller must ensure that getEmptyScope was successfully called. */
  uint32 getEmptyScopeShape() const { return emptyScope->shape; }

Comment 1

[Brendan Eich [:brendan]]

We could make empty scope for Array.prototype eagerly.

For 2, agree on OWN_SHAPE. Otherwise we could also make __proto__-setting eagerly make empty scope for new proto.

/be

Comment 2

[Brendan Eich [:brendan]]

Jason, can you take this? I'll get to it otherwise but I'd rather duck it and get back to 504587 and (especially) 497789.

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

Where are we on this?

Comment 4

[Jason Orendorff [:jorendorff]]

The patch in bug 505523 fixes this, I think. Confirming.

Comment 5

[Jason Orendorff [:jorendorff]]

It does.

(In reply to comment #1)
> For 2, agree on OWN_SHAPE. Otherwise we could also make __proto__-setting
> eagerly make empty scope for new proto.

The new code is simpler than either idea. If the proto's empty scope is a match, it'll use that shape. Otherwise it'll generate a new one.