549902 - JavaScript engine forgets base class when using with jquery.inherit-1.1.1.js

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Tagir Valeev]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2) Gecko/20100115 Firefox/3.6

Here's the test case:

<script type="text/javascript" src="jquery-1.4.2.js"></script>
<!-- http://code.google.com/p/jquery-inheritance/downloads/list -->
<script type="text/javascript" src="jquery.inherit-1.1.1.js"></script>
<script>
var Base = $.inherit({
	__constructor: function() {document.write("Base constructor<br>")}
});

function decorateFunc(f) {
	return function() {
		//this.__base; // this line does nothing, but fixes problem in FF3.6
		f.apply(this,arguments)
	}
}

var SubClass = $.inherit(Base, {
	__constructor: decorateFunc(
		function() {this.__base();document.write("SubClass constructor<br>")}
	)
});

try {
	new SubClass();
}
catch(e) {
	document.write(e);
}
</script>


Reproducible: Always

Steps to Reproduce:
1. Get jquery-1.4.2.js and jquery.inherit-1.1.1.js (URL in test case)
2. Copy test case from Details block to test.html and put into the same directory as jquery-1.4.2.js and jquery.inherit-1.1.1.js
3. Open test.html in FF3.6
Actual Results:  
TypeError: this.__base is not a function

Expected Results:  
What I see in Safari, IE, Chrome and what is correct:
Base constructor
SubClass constructor

Uncommenting //this.__base; line will magically make things correct in FF3.6 too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Which exact query-1.4.2.js are you using, just to make sure I do the same thing as you're doing when reproducing?

Comment 2

[Tagir Valeev]

Here's exactly the files I used:
http://lany.gorodok.net/mozbug/test.html
http://lany.gorodok.net/mozbug/jquery.inherit-1.1.1.js
http://lany.gorodok.net/mozbug/jquery-1.4.2.js 

Seems that jQuery lib version is of least concern. I tried also jquery-1.3.2.js, result is the same.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  This doesn't seem specific to Firefox 3.6.  I see the same behavior in Firefox 3.5, 3.0, and 2.0.  I'll attach the testcase in a sec.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Test file with all the JS inlined

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: js shell testcase

Still has a bunch of jquery gunk, but better!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ok, here's the key part in the jquery.inherit gunk:

		if(hasBase
			&& $.isFunction(basePtp[this]) && $.isFunction(props[this])
			&& (!hasIntrospection || props[this].toString().indexOf('.__base') > -1)) {

In the attached testcase, props[this] is indeed a function, and is specifically the function returned from decorateFunc.  The code guarded by that "if" statement is what sets up the __base property on the __constructor before calling it.

Note what this code is doing: it's taking the function in question (the one that's returned from decorateFunc, inspecting its toString() for the string ".__base", and if found setting up a __base property on the Function object it's about to call (which it makes up on the spot) before calling it.  If not found, it doesn't set up the property and calls a different function directly.  

In any case, in this testcase, the key is that toString() on a function preserves comments in at least V8, JSC, and whatever Opera 10 uses, but does NOT preserve them in spidermonkey.  Trivial testcase:

  javascript:alert(function() { /* foo */ });

Since this code is inspecting the toString of the function for the string ".__base", it will find it in those other browsers if the comment is present, but won't find it in Spidermonkey.  Of course what that means is that deleting the comment will make the code fail in those other browsers, as indeed it does (and you thought the code behavior changing when you commented out a no-op getter was weird?).

Looks to me like either the code in question is buggy or it's being misused (or both).  It seems to want to know whether calling the __constructor function will use __base, which is why it's doing the check above, but there are ways that a function call can use __base without it being present in the source.  The attached testcase using f.apply(this) is one way, but the __constructor could also just pass |this| to some totally different function that then got .__base from it, and so forth.  So I'm leaning toward "this code is buggy", unless it's clearly documented that __base is only supported in the actual __constructor function itself.

There's the separate question of whether Spidermonkey toString() should preserve comments, I guess.

Comment 7

[Tagir Valeev]

I see, thank you. Yes, removing comment at all breaks the code in other browsers too. My bad, I actually wasn't expected that comment may change the behavior. I will notify jquery.inherit-1.1.1.js author pointing to this problem.

Seems that preserving comments or not is implementation dependent. At least I cannot find specific recomendations about this in ECMA-262. So, I think, this can be marked as Resolved. Sorry for bothering you.

Comment 8

[Brendan Eich [:brendan]]

ECMA-262 underspecifies Function.prototype.toString and will not overspecify it to require comment preservation, even as we do add a few more specs (e.g., about named lambdas vs. function definitions).

This bug is INVALID. I'll let the reporter resolve it that way if everyone agrees.

/be