Closed Bug 863769 Opened 8 years ago Closed 8 years ago

ArrayBufferInputStream doesn't root its ArrayBuffer well

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g tef+
Tracking Status
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jdm, Assigned: jdm)

References

Details

(Keywords: regression, Whiteboard: [fixed-in-birch])

Attachments

(1 file, 2 obsolete files)

Andrew's seen it trigger crashes when it outlives its JSContext. We should use something like JS_AddNamedRoot or JS_AddValueRootRT instead.
This is required for making the email app stable after bug 831107.
blocking-b2g: --- → tef?
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?
Attachment #739869 - Flags: feedback?(bugmail)
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.
Attachment #739869 - Flags: feedback?(bugmail) → feedback+
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?
Attachment #739869 - Flags: review?(vladimir)
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?
Ah, right. I suppose storing a jsval would be a better choice.
This appears to work in tests.
Attachment #740026 - Flags: review?(vladimir)
Attachment #739869 - Attachment is obsolete: true
Attachment #739869 - Flags: review?(vladimir)
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.
One bug, one issue. This bug is about incorrect rooting, not about semantics.
Attachment #740026 - Flags: review?(jcoppeard)
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().
Attachment #740026 - Flags: review?(jcoppeard) → review+
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.
Attachment #740026 - Flags: review?(vladimir) → review+
The RootedObject stuff on central is likely going to need to change, since it uses a JSContext.
Blocks a blocker.
blocking-b2g: tef? → tef+
Keywords: regression
It seems like Josh is on this.
Assignee: nobody → josh
Attachment #740026 - Attachment is obsolete: true
Whiteboard: [status: has patch]
https://hg.mozilla.org/mozilla-central/rev/19510f91504b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.