Closed Bug 537673 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: fischer.th, Assigned: mrbkap)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

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)()
OS: Windows XP → All
Version: unspecified → 3.5 Branch
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: 3.5 Branch → 1.9.1 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
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'.
Summary: Wrong Javascript optimization in Trace Monkey → Calling an object property value evaluated inside a constant-folded conditional uses wrong 'this'
Suggest simple fix of not constant-folding any parenthesized callee expression.

/be
Excellent bug report and testcase.  :-)
Attached patch As brendan saidSplinter Review
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #420168 - Flags: review?(jorendorff)
Comment on attachment 420168 [details] [diff] [review]
As brendan said

>+        uint32 type = pn->pn_type;

Use JSTokenType, not uint32, for type.

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

That needs to be static_cast<>ed. Is it worth it?
(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
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);
  }
  ...
};
(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 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?
Attachment #420168 - Flags: review?(jorendorff) → review+
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.
Whiteboard: fixed-in-tracemonkey
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
http://hg.mozilla.org/mozilla-central/rev/981eb360a6cb
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 646695
You need to log in before you can comment on or make changes to this bug.