783396 - AnalyzeNewScriptProperties wrong for 'this.x = blah || this.x'

Status: RESOLVED DUPLICATE of bug 767961

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

This fails on trunk and aurora:

  function f(a) { this.z = false || this.z; }
  f.prototype.z = 42;
  assertEq(new f({}).z, 42);

because AnalyzeNewScriptProperties determines that 'z' is a definition property, so it eagerly adds it to the shape which means that the 'this.z' lookup sees 'undefined' instead of '42'.

This is the actual cause of bug 773929 and, when it is fixed, the '|| !fun->script()->enclosingStaticScope' hack-fix added in that bug can be removed.

Comment 1

[Brian Hackett [Laid off!]]

Thanks for reducing this!  Shu, can you look at this?  I think it's a regression from bug 759978.  I don't know if the problem is that the new script properties analysis should be rejecting this script, or that TypeConstraintClearDefiniteSetter should just invalidate new property info anytime the properties are also on the prototype (and not just when they are a setter / non-writable).

Comment 2

[Brian Hackett [Laid off!]]

Oops.  Shu, can you look at comment 1?

Comment 3

[Shu-yu Guo [:shu]]

tl;dr: Fixed with the other patch in 767961

The definite property analysis shouldn't have considered this.z a definite prop to begin with. The analysis isn't very sophisticated, and it bails on *any* use of 'this' (be it read or write) that is conditional. The bytecode for f in Luke's testcase and where the bail happens:

00000:  this
00001:  false
00002:  or 14 (+12)
00007:  pop
00008:  this
00009:  getprop "z" [use of 00008] <== definite property analysis bails here
00014:  setprop "z" [use of 00000]
00019:  pop
00020:  stop

Bug 759978 made the analysis a little more sophisticated and added a pending stack of pushed 'this' values. There was a bug where upon bailing, the analysis would process all pending 'this' values regardless of whether the entire function was fully analyzed, so the push of 'this' on 0000 would get processed and marked as a definite property.

For your edification, note that something like

  this.z = this.z

would also bail out the analysis and not mark this.z as a definite property, because the analysis bails out on any non-setprop (and call, but ignore that for this bug) uses of 'this'. This bug is observable only when the getprop of a property occurs _before_ its setprop, and the analysis bails on it without setting the property as definite.