Miscellaneous rooting issues in self-hosting infrastructure and Method JIT

RESOLVED FIXED in mozilla18



JavaScript Engine
5 years ago
5 years ago


(Reporter: Norbert Lindenberg, Unassigned)


Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [js:t])


(1 attachment)



5 years ago
Created attachment 666364 [details] [diff] [review]
patch with miscellaneous fixes

Using rooting verification mode on code for the ECMAScript Internationalization API, I found and fixed a few issues in other code (self-hosting support, JIT). I'd appreciate if someone more familiar with the modified code could take it from here.

Changes in the attached patch:

- a few API changes from jsid to HandleId and from Value* to MutableHandleValue, plus the required changes to callers,

- several additional Rooted<T> declarations and related changes.

For the latter I verified that they are required, i.e., undoing any of them reintroduced assertion failures.
Attachment #666364 - Attachment is patch: true
Comment on attachment 666364 [details] [diff] [review]
patch with miscellaneous fixes

Review of attachment 666364 [details] [diff] [review]:

Nice job!  Just a couple small nits.  I'll go ahead and apply them and get this in tree.

::: js/src/jscntxt.cpp
@@ +314,5 @@
>      return funVal.toObject().toFunction();
>  }
>  bool
> +JSRuntime::cloneSelfHostedValueById(JSContext *cx, HandleId id, HandleObject holder, Value *vp)

We should just make vp a MutableHandleValue here, rather than using result as a temporary.

::: js/src/methodjit/Compiler.cpp
@@ +78,5 @@
>      globalSlots(globalObj ? globalObj->getRawSlots() : NULL),
>      sps(&cx->runtime->spsProfiler),
>      masm(&sps, &PC),
>      frame(cx, *thisFromCtor(), masm, stubcc),
> +    a(NULL), outer(NULL), script_(cx, NULL), PC(NULL), loop(NULL),

Rooted will default to a sane value (NULL for T*), so we leave if off here.
Attachment #666364 - Flags: review+
Attachment #666364 - Flags: checkin?(terrence)
Blocks: 753203
Whiteboard: [js:t]
Attachment #666364 - Flags: checkin?(terrence) → checkin+

Should this have tests?
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 4

5 years ago
(In reply to Ryan VanderMeulen from comment #3)
> Should this have tests?

Reliably reproducing the assertion failures that prompted these changes requires a special SpiderMonkey build (--enable-root-analysis). I assume such a build isn't produced as part of regular build and test processes, so a test case wouldn't be very useful. In addition, the test case I used relies on a pile of changes that haven't been committed yet.
Flags: in-testsuite? → in-testsuite-
Actually, we run the full jit-test suite in the rooting analysis mode.  You can find it here: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&noignore=1&jobname=spidermonkey under 'r'.

This particular change is just moving to a new API, so the existing tests should cover this case, or new tests should get added on the bug covering the actual feature work.
You need to log in before you can comment on or make changes to this bug.