790108 - IonMonkey: ion::ArrayPushDense is not moving GC safe

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

I'm not sure if this affects anything yet, but ion::ArrayPushDense calls js::array_push with a fake-o argv. The argv is not rooted. The items in the argv are rooted, but we won't explicitly mark argv.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1: Use HandleObject, MutableHandleValue

Modifies the pop/shift/push stubs to take HandleObject and MutableHandleValue.

Comment 2

[Jan de Mooij [:jandem]]

I'm not sure how to proceed with part 2. GC people, what's the right way to root argv here (obj is a HandleObject and v a HandleValue):

    Value argv[] = { UndefinedValue(), ObjectValue(*obj), v };
    if (!js::array_push(cx, 1, argv))
        return false;

Similar-looking code in jsproxy.cpp uses AutoValueArray, is it okay to use it here or is it left over from the old GC rooters?

Comment 3

[Terrence Cole [:terrence]]

I fixed up something very similar in the jsapi-tests yesterday.

The answer is that we do not currently have a great answer.  It is finally possible to use RootedValue here: we now have an empty constructor which will use the TlsRuntime.  However, the inline initialization did not work when I tried it: I had to give an explicit size and split out the initializations onto separate lines.

Given the above, I think your best bet is to go with AutoValueArray.  Once we do get a good solution in place (possibly after we enable exact rooting), we are going to revisit all of the places where we're using AutoValueArray anyway.  We can fix it up at the same time as we fix the other uses.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2: Use AutoValueArray

Use AutoValueArray per comment 3 (thanks!).

Comment 5

[Daniel Veditz [:dveditz]]

Since we don't currently have a moving GC this isn't a current security issue; I don't want to give it a scary rating but I don't want to lose track of it. Do we have a meta bug for moving GC that we could make this block?

Comment 6

[Terrence Cole [:terrence]]

This is also an issue for exact stack rooting, so blocking on that.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 660363 [details] [diff] [review]
Part 2: Use AutoValueArray

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

Awesome!

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e84c9f521388
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ceaa28629d

Comment 9

[Jan de Mooij [:jandem]]

Sorry, closed this without thinking.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e84c9f521388
https://hg.mozilla.org/mozilla-central/rev/d7ceaa28629d