Closed Bug 873209 Opened 10 years ago Closed 10 years ago

Should root vp for specialized DOM getters

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file)

They certainly seem to assume it's rooted when they do things like:

  *vp = something;
  return JS_WrapValue(cx, vp);

right?
Comment on attachment 750631 [details] [diff] [review]
Trace the *vp of specialized DOM getters in Ion.

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

::: js/src/ion/CodeGenerator.cpp
@@ +6242,5 @@
>  
>      masm.checkStackAlignment();
>  
> +    /* Make space for the outparam.  Pre-initialize it to
> +       UndefinedValue so we can trace it at GC time. */

Pre-existing nit: //-style comment
Attachment #750631 - Flags: review?(jdemooij) → review+
> Pre-existing nit: //-style comment

Fixed.  Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1149f381ed and then backed it out: https://hg.mozilla.org/integration/mozilla-inbound/rev/989f42c1bc87

This was hitting the assert at the end of visitGetDOMProperty:

    JS_ASSERT(masm.framePushed() == initialStack);

The reason for that is that pushValue does NOT adjust the value returned by masm.framePushed().

Should it?  If not, should I be doing something else in this code?  Looking around, maybe what I really wanted was:

    masm.Push(UndefinedValue());

but I'd like to verify that before I push this again.
Flags: needinfo?(jdemooij)
(In reply to Boris Zbarsky (:bz) from comment #3)
> Should it?  If not, should I be doing something else in this code?  Looking
> around, maybe what I really wanted was:
> 
>     masm.Push(UndefinedValue());
> 
> but I'd like to verify that before I push this again.

Ah, yeah masm.Push(UndefinedValue()) is what you want.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e13bdaea06
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/f6e13bdaea06
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.