Last Comment Bug 670071 - make Invoke + InvokeArgsGuard less error prone
: make Invoke + InvokeArgsGuard less error prone
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-07 20:51 PDT by Brian Hackett (:bhackett)
Modified: 2011-08-19 03:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (42.33 KB, patch)
2011-08-04 11:53 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-07-07 20:51:05 PDT
Noticed this while debugging a test failure in bug 665815.  InvokeArgsGuard() explicitly invokes the default constructor for CallArgsList(), but there is no such constructor and the members of CallArgsList() (such as active_) are uninitialized until the Invoke() happens.
Comment 1 Luke Wagner [:luke] 2011-07-07 21:13:25 PDT
That is weird; I'll tidy up.  Btw, InvokeArgsGuard itself is initialized such that InvokeArgsGuard::pushed() returns false until pushInvokeArgs is called (before Invoke), so I don't think there is any actual bug here, yes?  (CallArgsList::active() only has meaning once the element has been pushed on the StackSegment::calls_ list).

As for the weirdness: CallArgsList used to have a default constructor, then that was removed b/c of some 'goto' business in JSOP_CALL (which itself was removed b/c LLVM didn't like it...), but this little bit of code was forgotten.  Thanks for pointing it out.
Comment 2 Luke Wagner [:luke] 2011-07-08 08:46:07 PDT
Also as a TODO here that Brian pointed out: the two Invoke overloads can be lead to subtle mistakes so perhaps the CallArgs-taking one which is (1) rare and (2) can be wrong should be renamed.  Case and point, I think InvokeConstructor calls the wrong overload since it takes an CallArgs&!  (This bug would manifest itself as native constructor calls not showing up in the debugger's call stack.)
Comment 3 Luke Wagner [:luke] 2011-08-04 11:53:25 PDT
Created attachment 550775 [details] [diff] [review]
fix

The fix here is to change Invoke to InvokeKernel in the naming style of EvalKernel: if you call this, you should know what you are doing.  Also, I fixed some long-standing strangeness in naming re: ExternalInvoke.  The "external" "internal" distinction is pretty weak and all ExternalInvoke really does is copy args to the stack for you, regardless of where the call came from.  This patch also fixes a real (jsdbgv2-facing) bug, as tested by the addition to testStackIter.js.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-15 09:16:15 PDT
Comment on attachment 550775 [details] [diff] [review]
fix

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

::: js/src/jsinterp.cpp
@@ +813,5 @@
>  }
>  
>  bool
> +InvokeConstructor(JSContext *cx, const Value &fval, uintN argc, Value *argv,
> +                        Value *rval)

Misaligned.

::: js/src/jsproxy.cpp
@@ +134,1 @@
>      }

Kill the braces.

@@ +1387,2 @@
>      *vp = rval;
>      return ok;

Might as well eliminate the rval middleman here, right?
Comment 6 Marco Bonardo [::mak] 2011-08-19 03:21:26 PDT
http://hg.mozilla.org/mozilla-central/rev/73aafae10571

Note You need to log in before you can comment on or make changes to this bug.