671947 - Unqualified function invocation uses the global object the property was gotten from as |this|

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

See https://bugs.webkit.org/show_bug.cgi?id=64250 and kudos to Gavin and Oliver for blazing the trail toward ES5+multiple-globals righteousness.

We should try again, not so late in a cycle (with our new fast releases, this means nightly now targeting IINM Firefox 8), and evangelize any site or other content that breaks.

The V8 issue is http://code.google.com/p/v8/issues/detail?id=1547.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: patch in progress

Needs the testcase. I have a js shell test based on Gavin Barraclough's version of the one I hacked up from Mark Miller's original HTML testcase.

It may be possible to minimize the patch further. This one does pass the test.

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't get it.

In strict mode, we don't have the bug that JSC had there, right?  If I modify the testcase in bug 634590 to use a strict-mode function then |this| is undefined.

So is this bug about changing the behavior of non-strict-mode functions?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> In strict mode, we don't have the bug that JSC had there, right?

In strict mode *where*? The test covers strict caller and strict callee. The issue for us is not strict callee (you always get 'u' as the returned char).

> If I
> modify the testcase in bug 634590 to use a strict-mode function then |this|
> is undefined.

Yes, that is not in dispute (at least not yet -- IE10pp still gets that wrong but that is just a bug, and the ES5 spec is black letter law when it comes to strict callee |this| binding).

> So is this bug about changing the behavior of non-strict-mode functions?

Non-strict callers -- yes, of course.

/be

Comment 4

[Brendan Eich [:brendan]]

Waldo wanted to stick with ES5 in the Fx4 run-up, but it was not the time to break things. His desire is on target, though. ES5 is simpler, faster, and ocap secure. We are trying for a breaking change here. They've happened in the past, sometimes by accident. Better with intentional support.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 6

[Brendan Eich [:brendan]]

The workaround, of course, is to call with an explicit object.property() form of the callee expression. Instead of f() vs, (1, f)() having different |this| bindings, we fix the bug so they get the same |this| (the callee decides if it is non-strict to replace undefined with its global). If you want the caller's global because f is a property of that object, use this.f() or window.f().

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  Seems like we should message that far and wide now-ish, given that we _know_ there's content depending on this behavior...

It still bothers me that we're intentionally changing something that works interoperably across all browsers.  :(

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Two js1_8_5 tests needed tweaking.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> OK.  Seems like we should message that far and wide now-ish, given that we
> _know_ there's content depending on this behavior...
> 
> It still bothers me that we're intentionally changing something that works
> interoperably across all browsers.  :(

Only Firefox (after Netscape) and IE did this anywhere near interoperably over the long haul. Chrome 12 and Opera 11.50 return 'i' not 'o' (and doesn't do ES5 yet). Safari JSC of course is changing, but not so long ago they did something incompatible, I'm told.

This is not a place where all the browsers have agreed for long, unless by "all" you mean IE and Mozilla. I agree that's a concern, but it isn't clear how big the compatibility constraint is given Chrome and Safari (until recently) differing from the top two.

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Chrome 12 and Opera 11.50 return 'i' not 'o'

The testcase in bug 634590 comment 7 is interoperable in all browsers for a very liberal meaning of all (Chrome, Safari, Opera, us, IE).

I realize that there are other similar situations that are not interoperable.  But that particular situation _is_....  or am I missing something?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> > Chrome 12 and Opera 11.50 return 'i' not 'o'
> 
> The testcase in bug 634590 comment 7 is interoperable in all browsers for a
> very liberal meaning of all (Chrome, Safari, Opera, us, IE).
> 
> I realize that there are other similar situations that are not
> interoperable.  But that particular situation _is_....  or am I missing
> something?

No, you're right -- that one is the hard case. If it used window.f() instead of f(), it would work with this bug's patch. It may be not much content counts on the old behavior. It may be we can evangelize. Since JSC is trying to conform to ES5, we should too, and compare notes. Ideally V8 will match in nightlies of Chrome. We'll be talking to IE folks at the upcoming TC39 meeting.

/be

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 546265 [details] [diff] [review]
proposed fix, v2

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

Great change!  I am happy to see the expected results for cross-global-implicit-this.js.

::: js/src/jsinterpinlines.h
@@ +211,5 @@
>   * Compute the implicit |this| parameter for a call expression where the callee
>   * is an unqualified name reference.
>   *
>   * We can avoid computing |this| eagerly and push the implicit callee-coerced
>   * |this| value, undefined, according to this decision tree:

Enough time has passed since I r+'d the last patch that I got to rediscover the magic of 'this' and re-understand this comment.  I think, in the new simplified world, we can simplify the comment as well.  How about something roughly like:

Dynamic name lookup found the callee 'funval' on 'obj'. This function computes the 'this' value to be passed when calling 'funval'. According to ES? ?.?.?, the correct 'this' value is:
 1. 'obj', if 'obj' is a 'with' object;
 2. 'undefined', if funval is a strict-mode function or not a function at all;
 3. the global of the callee's scope chain, otherwise.

As an optimization, JSOP_THIS in a non-strict function coerces 'undefined' to the global of the current scope chain. Thus case 3 also computes 'undefined'.

To discriminate case 1 from 2 and 3, the predicate IsCacheableNonGlobalScope is used. This predicate returns 'true' for normal lexical scope objects (Call, Block, DeclEnv) and 'false' for all others. As a side-effect, other types of objects on the scope chain (introduced through the JSAPI) get treated like 'with' objects.

::: js/src/tests/ecma_5/Global/cross-global-implicit-this.js
@@ +49,5 @@
> +       ' results += (function(){return eval("f()");})();\n' +
> +       ' /* Same cases as above, but wrapped in a with. The first & last of these cases pass b, */\n' +
> +       ' /* the object scoped by the with, as the this value. */\n' +
> +       ' /* a.f() still passes the explicit base, a. (1,f)() is a little tricksier - this passes */\n' +
> +       ' /* undefined (promoted to the callee global object) since teh comma operator calles GetValue */\n' +

teh

Comment 13

[Luke Wagner [:luke]]

This is probably followup material (with a comment in this patch for future readers): I think generateGlobalStub can be optimized to not guard on the callee's global.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> This is probably followup material (with a comment in this patch for future
> readers): I think generateGlobalStub can be optimized to not guard on the
> callee's global.

Happy to do it here, just need a few minutes!

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: proposed fix, v3

I left the big comment more as before, but folded in good feedback from your last review -- thanks.

My reading of methodjit/Compiler.cpp jsop_this says this is correct, and tests all like it, but I need close reviewer attention.

/be

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 546677 [details] [diff] [review]
proposed fix, v3

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

Comment 17

[Brendan Eich [:brendan]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bedd0f79b82d

/be

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/bedd0f79b82d

Comment 19

[Jason Orendorff [:jorendorff]]

Brendan, this changeset added an entry to ecma_5/Global/jstests.list:

@@ -1,5 +1,6 @@
 url-prefix ../../jsreftest.html?test=ecma_5/Global/
 fails-if(!xulRuntime.shell) script adding-global-var-nonextensible-error.js
+fails-if(!xulRuntime.shell) script cross-global-implicit-this.js
 script parseInt-01.js
 script parseFloat-01.js

but it didn't add the file. Check it in?

Comment 20

[Brendan Eich [:brendan]]

Sorry!

http://hg.mozilla.org/integration/mozilla-inbound/rev/735e1dc41b44

Checkin message references bug 588061 by mistake, comment there points here.

/be

Comment 21

[Marco Bonardo [:mak]]

yay! Found the right bug!
http://hg.mozilla.org/mozilla-central/rev/735e1dc41b44

Comment 22

[Bob Clary [:bc] (inactive)]

brendan: since evalcx is shell only, shouldn't you just skip the test cross-global-implicit-this.js in the browser rather than mark it as a fails?

Comment 23

[Eric Shepherd [:sheppy]]

Is there a blog post or anything explaining what this impact is? I could use more insight into this change before I start writing.

Comment 24

[Eric Shepherd [:sheppy]]

evilpie documented this, so we're good to go. The documentation was already correct but we now have a note mentioning that it wasn't always done properly.