636364 - Followup fixes and tests in wake of global |this| binding change (shell failure on js1_8_5/regress/regress-555246-1.js)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[David Mandelin [:dmandelin]]

Bisected to:

changeset:   62529:4e085ba15d4c
user:        Andreas Gal <[e-mail]>
date:        Thu Feb 17 17:52:55 2011 -0800
summary:     Unqualified function invocation doesn't use the global object the property was gotten from as |this| (bug 634590, r=brendan).

It might be a bug in the test case. It doesn't look like the most important test ever, but we should get back to no failures on tip one way or another.

Comment 1

[Jeff Walden [:Waldo]]

Brendan, looks like you granted review on tests which explicitly required the behavior we had before bug 634590:

http://hg.mozilla.org/tracemonkey/rev/e4364736e170

It looks like the test expects the global for the function being called, when it has to compute this.

Comment 2

[Brendan Eich [:brendan]]

Yes, I like the ES5 semantics. A "use strict"; appeared before my eyes as I read function f.

Will fix and add a test for the strict-mode case (which we botch same-compartment currently as Luke pointed out!).

/be

Comment 3

[Brendan Eich [:brendan]]

This is more than a test that needs tweaking. It is a required followup to fix problems not fully fixed for bug 634590, with yet more tests.

Tests are good.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix with tests

Comment 5

[Brendan Eich [:brendan]]

Attachment: diff -w version of patch

David, could you please review the methodjit changes? Thanks,

/be

Comment 6

[Brendan Eich [:brendan]]

Thanks to Waldo for the newGlobal shell code. Its name goes against the more Unix-command-like lowercase names of shell functions, though. Any objection to my changing it to newglobal?

/be

Comment 7

[Andreas Gal :gal]

Comment on attachment 514714 [details] [diff] [review]
proposed fix with tests

newGlobal or newglobal both seem ok based on previous examples, really up to you

Comment 8

[Luke Wagner [:luke]]

(In reply to comment #6)
> Its name goes against the more
> Unix-command-like lowercase names of shell functions, though. Any objection to
> my changing it to newglobal?

To be really Unix-y, shouldn't you shave off a couple of characters off the end of 'global' too ;-)

>+        if (!rval.isObject() ||
>+            (thisp->isGlobal()
>+             ? IsSafeForLazyThisCoercion(cx, &rval.toObject())
>+             : IsCacheableNonGlobalScope(thisp))) {
>+            thisv.setUndefined();

This expression took me a long time to understand, and now there are two copies of it.  What do you think about:
 1. Share/reuse ComputeThis
 2. Break the compound expression into separate statements, with comments (in particular, seeing "Cacheable" mentioned is initially confusing)
 3. Kill the separate IsSafeForLazyThisCoercion predicate (whose name only half tells the story of what it does) and roll it into ComputeThis so that it can be explained in context

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #6)
> > Its name goes against the more
> > Unix-command-like lowercase names of shell functions, though. Any objection to
> > my changing it to newglobal?
> 
> To be really Unix-y, shouldn't you shave off a couple of characters off the end
> of 'global' too ;-)

Nah. Remember, ken admitted he should have spelled it "create". Plus, "glob" means shell wildcard in Unixese.

> >+        if (!rval.isObject() ||
> >+            (thisp->isGlobal()
> >+             ? IsSafeForLazyThisCoercion(cx, &rval.toObject())
> >+             : IsCacheableNonGlobalScope(thisp))) {
> >+            thisv.setUndefined();
> 
> This expression took me a long time to understand, and now there are two copies
> of it.

In the fine spirt of methodjit/StubCalls.cpp :-/.

> What do you think about:
>  1. Share/reuse ComputeThis

Can do.

>  2. Break the compound expression into separate statements, with comments (in
> particular, seeing "Cacheable" mentioned is initially confusing)

I was trying to minimize change at this point, but ok.

>  3. Kill the separate IsSafeForLazyThisCoercion predicate (whose name only half
> tells the story of what it does) and roll it into ComputeThis so that it can be
> explained in context

Don't want too many thisv.setUndefined() calls. I will do my best, stay tuned.

/be

Comment 10

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> Its name goes against the more Unix-command-like lowercase names of shell
> functions, though.

I wrote it, do I get a vote?  :-)

I'm seeing an awful lot of camelCaps in the current listings (dateNow, revertVersion, assertEq, dumpHeap, evalInFrame, etc.).  It seems to be the direction most changes have gone recently (without checking, mjitstats looks like the only outlier).  Unix getslx/getpda/putstr/cvtargs trsnss is but the learner now, camelCaps is the master.

Comment 11

[Brendan Eich [:brendan]]

Attachment: luke made me use the force

Luke: need your review, this is based on your good requests. gal and I have clearly collapsed into a slightly corrupt peer review clique.

Waldo: I left newGlobal spelled that way, no big.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: diff -w version of patch

Carry over dvander's r+ on methodjit changes.

/be

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 514821 [details] [diff] [review]
luke made me use the force

Righteous comments and renamings; much appreciated.

Comment 14

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/6e2ee1f3f6c6

/be

Comment 15

[Brendan Eich [:brendan]]

Comment tweaks:

http://hg.mozilla.org/tracemonkey/rev/03c4b2e3d12d
http://hg.mozilla.org/tracemonkey/rev/8aa277ade98d

/be

Comment 16

[Phil Ringnalda (:philor)]

This brought back the same thing we had over the weekend, massive make check failures on OS X 10.6 and total failure on OS X 10.5. Tree's closed as a result.

Comment 17

[Andreas Gal :gal]

I pushed a hotfix that tries to force no inlining on mac. We will see if this works.

http://hg.mozilla.org/tracemonkey/rev/199117e5cbd5

Comment 18

[Phil Ringnalda (:philor)]

Like a charm, tree's reopened.

Comment 19

[Brendan Eich [:brendan]]

Fach Apple's gcc fork.

/be

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6e2ee1f3f6c6