Closed Bug 670071 Opened 8 years ago Closed 8 years ago

make Invoke + InvokeArgsGuard less error prone

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: bhackett, Assigned: luke)

Details

(Whiteboard: [inbound])

Attachments

(1 file)

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.
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.
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.)
Summary: InvokeArgsGuard constructor leaves CallArgsList members uninitialized → make Invoke + InvokeArgsGuard less error prone
Attached patch fixSplinter Review
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #550775 - Flags: review?(jwalden+bmo)
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?
Attachment #550775 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/mozilla-central/rev/73aafae10571
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.