Tricky inheritance issue with FF 29

jambonnade commented 10 years ago

Hi,

We have a bug that appeared with Firefox 29 : some property in one object is lost when calling a redefined method. The conditions to reproduce are very particular but I identified them. Maybe it may lead to identify a bigger problem.

Here is the code

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="fr" lang="fr">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <title>Title</title>
    <script src="http://ajax.googleapis.com/ajax/libs/prototype/1.7.2.0/prototype.js"></script>
</head>
<body>
    <div><textarea id="log" rows="10" cols="80"></textarea></div>
    <p><button type="button" id="btn">demo</button></p>
    <script type="text/javascript">/* <!-- */

        var SuperClass = Class.create({
            'initialize': function () {
                this.data1 = 'a';
                this.data2 = 'b';

                // My "fix" is to make the browser fetch the properties with
                // this, it seems the bug doesn't appear then
                //Object.keys(this);

                this.log('init');
                //this.log('data3 (0) : "' + this.data3 + '"');

                // meth1 adds data3 in SubClassA
                this.meth1();
                this.log('data3 (1) : "' + this.data3 + '"');

                // meth2 erases data3 in SubClassA (bug)
                this.meth2();
                this.log('data3 (2) : "' + this.data3 + '"');

                this.log('\n');
            },

            'meth1': function () {
                this.log('meth1');
            },
            'meth2': function () {
                this.log('meth2');
                // meth2 needs to add data in SuperClass to trigger the bug
                this.data4 = 'x';
            },
            'log': function (stuff) {
                $('log').value += stuff + "\n";
            }
        });

        var SubClassA = Class.create(SuperClass, {
            'meth1': function () {
                this.log('meth1 (sub)');
                this.data3 = 'z';
            },
            'meth2': function ($super) {
                // meth2 needs to be redefined to trigger the bug
                this.log('meth2 (sub with super())');
                $super();
            }
        });

        var SubClassB = Class.create(SuperClass, {
            // Whatever here
        });

        $('btn').observe('click', function () {
            // Need to init another object from another subclass to trigger
            // the bug.
            new SubClassB();
            // Bugged object :
            new SubClassA();
        });

    /* --> */</script>
</body>
</html>

jambonnade commented 10 years ago

I forgot to mention : doesn't happen if firebug (or dev console) is opened. I believe firebug accesses the created objects in some way ; that's why, i added "Object.keys()" to fix, it may do the same kind of stuff internally.

Yaffle commented 10 years ago

@Gruikmusic , interesting bug, did you post a report at bugzilla.mozilla.org ? i can reproduce this without prototype.js :

<script>
function SuperClass() {
  this.meth1();
  var z = this.data3;
  this.meth2();
  if (z !== this.data3) {
    alert('bug!');
  }
}
SuperClass.prototype.meth1 = function () {
};
SuperClass.prototype.meth2 = function () {
  this.data4 = 'x';
};
var F = function () {
};
F.prototype = SuperClass.prototype;
function SubClassA() {
  SuperClass.call(this);
}
SubClassA.prototype = new F();
SubClassA.prototype.meth1 = function () {
  this.data3 = 'z';
};
SubClassA.prototype.meth2 = function () {
};
function SubClassB() {
  SuperClass.call(this);
}
SubClassB.prototype = new F();
new SubClassB();
new SubClassA();
</script>

jambonnade commented 10 years ago

i prefered to let you (prototypejs contributors) report to mozilla when found whether it's related to firefox or prototypejs

Yaffle commented 10 years ago

@Gruikmusic , so you have to wait prototypejs contributors to do this.... Anyway, i think, it is not good, that common constructor creates different properties on an object, because of redefined "meth1" and "meth2"...

jambonnade commented 10 years ago

I reported to mozilla https://bugzilla.mozilla.org/show_bug.cgi?id=1008339

About derived methods in constructor, I'm used to do this in PHP and I find it interesting :

class Parent
{
  public function __construct($params)
  {
     // [...]

     $this->_initSomeStuff();
     $this->_initOtherStuff();

     // [...]
  }
  protected function _initSomeStuff()
  {
    // default code
  }
  protected function _initOtherStuff()
  {
    // default code
  }
}

_initSomeStuff and _initOtherStuff may be overridden then in subclasses to customize some parts of initialization while keeping a common construction (and a common constructor signature)

Yaffle commented 10 years ago

I reported to mozilla https://bugzilla.mozilla.org/show_bug.cgi?id=1008339

good. ;

About derived methods in constructor, I'm used to do this in PHP and I find it interesting : Thats OK.

But in javascript the sequence of property assignments affects the "class" of an object. So, it is better to define all properties in the constructor.

savetheclocktower commented 9 years ago

Looks like Firefox fixed this, which is good, because I wouldn't have known where to start. Thanks!

prototypejs / prototype

Tricky inheritance issue with FF 29 #195