863769 - ArrayBufferInputStream doesn't root its ArrayBuffer well

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Josh Matthews [:jdm]]

Andrew's seen it trigger crashes when it outlives its JSContext. We should use something like JS_AddNamedRoot or JS_AddValueRootRT instead.

Comment 1

[Josh Matthews [:jdm]]

This is required for making the email app stable after bug 831107.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 739869 [details] [diff] [review]
Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

Does this improve the situation, Andrew?

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 739869 [details] [diff] [review]
Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

Yes, multiple complete unit tests runs pass without the dramatic crashing that would reliably happen before.

It looks like API-wise it might be better to continue to use a CX-using rooter function rather than js_AddRootRT (which is marked as "TODO: remove these APIs" in jsapi.h), but then remove the root using just the runtime using JS_RemoveValueRootRT or something like it.  However, all of the API calls end up going to the same place within 1-line of code, so it does seem a bit pedantic.

There is some urgency about landing these fixes plus that correctness fix you provided (at http://www.joshmatthews.net/ppp), at least on b2g18 and b2g18_v1_0_1.  I'll try and get more proper approval, but otherwise, I may end up trying to land these on the tef+ nature of the initial bug and the assumption that my JS-fu and observing things no longer crashing is good enough.

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 739869 [details] [diff] [review]
Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

What say you, Vlad?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 739869 [details] [diff] [review]
Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

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

::: netwerk/base/src/ArrayBufferInputStream.cpp
@@ +37,5 @@
>    if (!aBuffer.isObject()) {
>      return NS_ERROR_FAILURE;
>    }
> +  JSObject* arrayBuffer = &aBuffer.toObject();
> +  mArrayBuffer = const_cast<jsval*>(&aBuffer);

So you're going to try too keep a reference to a jsval on the interpreter stack, that's going to be overwritten as soon as this function returns?

Comment 7

[Josh Matthews [:jdm]]

Ah, right. I suppose storing a jsval would be a better choice.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

This appears to work in tests.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Can we land the semantics fix at http://www.joshmatthews.net/ppp along-side this patch?  It's required for the e-mail app to work too.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

One bug, one issue. This bug is about incorrect rooting, not about semantics.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 740026 [details] [diff] [review]
Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

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

::: netwerk/base/src/ArrayBufferInputStream.cpp
@@ +38,5 @@
>      return NS_ERROR_FAILURE;
>    }
> +  JSObject* arrayBuffer = &aBuffer.toObject();
> +  mArrayBuffer = aBuffer;
> +  js_AddRootRT(mRt, &mArrayBuffer, "mArrayBuffer");

The js_AddRootRT() API has been removed on central.  Please replace this with JS_AddNamedValueRootRT(), which is preferable anyway  as this provides a name which can help with debugging.  Also the destructor needs similarly updating to use JS_RemoveValueRootRT().

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 740026 [details] [diff] [review]
Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

Yep, what Jon said -- JS_AddNamedValueRootRT is preferable.  However, this code is already landed differently on central (using ObjectRoot), so it's likely moot.

Comment 13

[Josh Matthews [:jdm]]

The RootedObject stuff on central is likely going to need to change, since it uses a JSContext.

Comment 14

[Dietrich Ayala (:dietrich)]

Blocks a blocker.

Comment 15

[Andrew Overholt [:overholt]]

It seems like Josh is on this.

Comment 16

[Josh Matthews [:jdm]]

Attachment: Make ArrayBufferInputStream use rooting APIs that don't rely on the JS context being valid.

Fixed up b2g18 patch.

Comment 17

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f2c7fdc2fb56
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d794f50ca455
https://hg.mozilla.org/projects/birch/rev/19510f91504b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19510f91504b