537673 - Calling an object property value evaluated inside a constant-folded conditional uses wrong 'this'

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Thomas Fischer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.6) Gecko/20091215 Ubuntu/9.10 (karmic) Firefox/3.5.6

Trace Monkey "optimize" Javascript code in a wrong way.

Reproducible: Always

Steps to Reproduce:
1. Open the Firebug console
2. Input and execute following code
   myName = "win";
   getName = function(){return this.myName};
   obj = {myName: "obj", getName: getName};
   val = true;
   console.info((val  ? obj.getName : 4711)()); 
   console.debug((true ? obj.getName : 4711)());
Actual Results:  
console output is:
win
obj

Expected Results:  
console output should be:
win
win

Presumption:
Trace Monkey "optimize" the code:
   (true ? obj.getName : 4711)()
into:
   obj.getName()
instead of:
   (obj.getName)()

Comment 1

[David Mandelin [:dmandelin]]

dis() shows that the first conditional expression is compiled with a branch that uses JSOP_GETPROP to get the property, while the second conditional is constant-folded and then compiled with JSOP_CALLPROP, so it uses the object 'this' when it apparently should be global 'this'.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Shell test case

Comment 3

[Brendan Eich [:brendan]]

Suggest simple fix of not constant-folding any parenthesized callee expression.

/be

Comment 4

[Jeff Walden [:Waldo]]

Excellent bug report and testcase.  :-)

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: As brendan said

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 420168 [details] [diff] [review]
As brendan said

>+        uint32 type = pn->pn_type;

Use JSTokenType, not uint32, for type.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> Use JSTokenType, not uint32, for type.

That needs to be static_cast<>ed. Is it worth it?

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #6)
> > Use JSTokenType, not uint32, for type.
> 
> That needs to be static_cast<>ed. Is it worth it?

Fooey, C-style cast!

Or try this: the temporary is unnecessary. Let the compiler optimize. No need for extra lines and locals. We use temps all over, true, old code from dark ages of compilers (not just lack of strict aliasing, plain bad register allocation -- possibly still with us in some GCCs!), also (now probably bad) habit.

/be

Comment 9

[Jeff Walden [:Waldo]]

Temps aren't just to help out compilers, they're to avoid overhead of thinking about things like "will this expression's value change" and to abbreviate when a common phrase might have to be repeated.  Both these seem to be the case here, and I don't see over-optimization coming into the picture.

But maybe we should we have this instead of worrying about casts or semantically-mismatching types?

struct JSParseNode {
  ...
  JSTokenType type() const {
      uint32 t = pn_type;
      JS_ASSERT(TOK_EOF <= t && t < TOK_LIMIT);
      return JSTokenType(t);
  }
  ...
};

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Temps aren't just to help out compilers, they're to avoid overhead of thinking
> about things like "will this expression's value change" and to abbreviate when
> a common phrase might have to be repeated.

Hey, you know I love temps -- js/src is festooned with 'em. But apart from multi-threaded access, there are no such hazards here -- no call-outs that pass anything connected to the node. Plus, AST nodes have immutable type :-P.

> Both these seem to be the case
> here, and I don't see over-optimization coming into the picture.

Let's let mrbkap defend himself, since he knows why he wrote it this way :-/.

> But maybe we should we have this instead of worrying about casts or
> semantically-mismatching types?
> 
> struct JSParseNode {
>   ...
>   JSTokenType type() const {
>       uint32 t = pn_type;
>       JS_ASSERT(TOK_EOF <= t && t < TOK_LIMIT);
>       return JSTokenType(t);
>   }

That's great, we should have it. Not as big a win as avoiding BOGUS_CX but it helps. And I see no static_cast blarny -- bonus!

/be

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 420168 [details] [diff] [review]
As brendan said

Just so it doesn't look so much like a bug, can you move the `pn1 = pn->pn_head` up and initialize pn1 at the same time as pn2?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/981eb360a6cb

I didn't intend to over-optimize. Previously, style in js/src asked to common out repeated expressions. I've left it repeated for now.

Comment 13

[David Mandelin [:dmandelin]]

Looks like this is causing assertions on Mochitest. E.g.

  http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1263252639.1263252939.32018.gz

  Assertion failure: pn2->pn_type == TOK_NAME, at e:/builds/moz2_slave/tracemonkey-win32-debug/build/js/src/jsemit.cpp:6101

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/981eb360a6cb