1008339 - prototype-based inheritance : calling derived method in constructor erases properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[gruikmusic]

Attachment: ff29-bug-demo.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

Hi,

This weird bug appeared just when FF29 came out ; I first submitted the problem to prototype.js ( https://github.com/sstephenson/prototype/issues/195 ) because this happened by using prototype.js object API, but someone there gave me a code showing it happens without using prototype.js.

The conditions to trigger the bug are very particular : we need a derived method called in the parent constructor that adds properties, that method in the parent class needs to add properties too (even if the parent method is not called), and we need to instantiate another subclass of the parent class before the one showing the case.

In the demo I log stuff in a textarea because with the demo i made using prototype.js, the bug didn't occur if firebug or developer console was opened.

The way to implement inheritance is the old way (like here http://phrogz.net/JS/classes/OOPinJS2.html ) : setting "prototype" property of child constructors to a new instance of the parent class (except we use an intermediate "empty" constructor to avoid executing the parent constructor code on instantiating children prototype ; but the bug happens too if we instantiate Parent directly).
If we use Object.create(), it works fine.




Actual results:

log :


ChildB ctor
Parent ctor
Parent.meth1()
data3 before : undefined
Parent.meth2()
data3 after : undefined

ChildA ctor
Parent ctor
ChildA.meth1()
data3 before : z
ChildA.meth2()
data3 after : undefined



Expected results:

log :


ChildB ctor
Parent ctor
Parent.meth1()
data3 before : undefined
Parent.meth2()
data3 after : undefined

ChildA ctor
Parent ctor
ChildA.meth1()
data3 before : z
ChildA.meth2()
data3 after : z

Comment 1

[gruikmusic]

I messed up in explaining the requirement to fulfill, well read the code to see all the requirements

Comment 2

[:Gijs (he/him)]

This reproduces on 32 on OS X as well, and seems to work in V8. Over to the JS engine...

Comment 3

[Logan Rosen [:Logan]]

Last good revision: 1e7ec87921a5
First bad revision: 53a3cde703de
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e7ec87921a5&tochange=53a3cde703de

Only one commit: http://hg.mozilla.org/integration/mozilla-inbound/rev/53a3cde703de

Blocking the associated Bug 936156.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

Thanks for the great test case!  This predates bug 936156, actually.  The problem is that we thought that data4 was a property that ChildA objects definitely have and added it at the object's creation point, but when ChildA.meth1 was called first we didn't properly invalidate that information and ended up inadvertently erasing the data3 property when the definite property information was invalidated at the call to ChildA.meth2.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8420591 [details] [diff] [review]
patch

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

r=me with comments below addressed.

::: js/src/jit-test/tests/basic/bug1008339.js
@@ +1,2 @@
> +
> +function Parent() {

Shouldn't the test have some assertEq's in them? Please make sure it fails without the patch.

::: js/src/jit/IonBuilder.cpp
@@ +4194,5 @@
>          return false;
>  
> +    // Don't inline polymorphic sites during the definite properties analysis.
> +    // AddClearDefiniteFunctionUsesInScript depends on this for correctness.
> +    if (info().executionMode() == DefinitePropertiesAnalysis && targets.length() > 0)

targets.length() > 1

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> ::: js/src/jit-test/tests/basic/bug1008339.js
> @@ +1,2 @@
> > +
> > +function Parent() {
> 
> Shouldn't the test have some assertEq's in them? Please make sure it fails
> without the patch.

We actually busted on this test with an assert in debug builds, when we noticed we were erasing a property that didn't have an undefined value.

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/582e943a30a5

Comment 8

[Mayank Bansal]

this appears to have caused a 9.7% regression (32034->28933)  on Octane-DeltaBlue on machine 11 on AWFY.

Regression range from AEFY : http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4e1e052624c2&tochange=14450e205899

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/582e943a30a5

Comment 10

[Brian Hackett [Laid off!]]

(In reply to mayankleoboy1 from comment #8)
> this appears to have caused a 9.7% regression (32034->28933)  on
> Octane-DeltaBlue on machine 11 on AWFY.
> 
> Regression range from AEFY :
> http://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=4e1e052624c2&tochange=14450e205899

Thanks!  I filed bug 1009899 for the regression.