The default bug view has changed. See this FAQ.

make Invoke + InvokeArgsGuard less error prone

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Assigned: luke)

Tracking

unspecified
mozilla9
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

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.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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.)
(Assignee)

Updated

6 years ago
Summary: InvokeArgsGuard constructor leaves CallArgsList members uninitialized → make Invoke + InvokeArgsGuard less error prone
(Assignee)

Comment 3

6 years ago
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.
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+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/73aafae10571
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/73aafae10571
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.