Last Comment Bug 765168 - XHR.send shouldn't take a jscontext
: XHR.send shouldn't take a jscontext
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
Depends on:
Blocks: 754202
  Show dependency treegraph
 
Reported: 2012-06-15 02:59 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-06-21 04:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Remove the dependency of XHR::send on a cx parameter. v1 (2.94 KB, patch)
2012-06-15 04:00 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 2 - Remove js context parameter from XHR.send call chain. v1 (12.48 KB, patch)
2012-06-15 04:00 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 1 - Remove the dependency of XHR::send on a cx parameter. v2 (3.02 KB, patch)
2012-06-18 08:30 PDT, Bobby Holley (:bholley) (busy with Stylo)
bugs: review-
Details | Diff | Splinter Review
Part 2 - Remove js context parameter from XHR.send call chain. v2 (14.22 KB, patch)
2012-06-18 08:30 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 2 - Remove js context parameter from XHR.send call chain. v3 (14.46 KB, patch)
2012-06-18 08:53 PDT, Bobby Holley (:bholley) (busy with Stylo)
bugs: review+
Details | Diff | Splinter Review
Part 1 - Remove the dependency of XHR::send on a cx parameter. v3 (3.35 KB, patch)
2012-06-20 02:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
bugs: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-06-15 02:59:03 PDT
AFAICT, it's passed all over the place, and only ever used here:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2686

I think we can do something better.
Comment 1 Olli Pettay [:smaug] (reviewing overload) 2012-06-15 03:03:04 PDT
It is used in GetRequestBody
Comment 2 Olli Pettay [:smaug] (reviewing overload) 2012-06-15 03:03:37 PDT
Er, nm.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-06-15 04:00:37 PDT
Created attachment 633472 [details] [diff] [review]
Part 1 - Remove the dependency of XHR::send on a cx parameter. v1

Given the new bindings, I think this is dead code anyway modulo the pref.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-06-15 04:00:52 PDT
Created attachment 633473 [details] [diff] [review]
Part 2 - Remove js context parameter from XHR.send call chain. v1
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 07:54:54 PDT
> Given the new bindings, I think this is dead code anyway modulo the pref.

Even for the createInstance case?  If so, we should most certainly remove the pref and nuke all this code as well as the relevant XPCOM interfaces!

The |if (cx)| check sure looks backwards to me.
Comment 6 Olli Pettay [:smaug] (reviewing overload) 2012-06-15 08:42:21 PDT
I don't think we use new bindings with createInstance, since in that case XHR isn't parented to
anything (GetParentObject() returns null).
Comment 7 Peter Van der Beken [:peterv] 2012-06-15 12:50:31 PDT
(In reply to Olli Pettay [:smaug] from comment #6)
> (GetParentObject() returns null).

That doesn't matter, we always use the new bindings if the pref is enabled.
Comment 8 Olli Pettay [:smaug] (reviewing overload) 2012-06-15 13:45:53 PDT
Oh, I thought in new bindings world wrappercached objects need to have some parent. Apparently no.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 13:53:12 PDT
We changed that, for certain classes of use cases.
Comment 10 Blake Kaplan (:mrbkap) 2012-06-15 13:58:57 PDT
Comment on attachment 633472 [details] [diff] [review]
Part 1 - Remove the dependency of XHR::send on a cx parameter. v1

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2681,5 @@
>      jsval realVal;
>      JSObject* obj;
> +    JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +    if (cx)
> +      cx = nsContentUtils::GetSafeJSContext();

Sorry for the drive-by review, but I think you need to push cx here so we can see elsewhere that we are actually running JS code here.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-06-18 08:30:17 PDT
Created attachment 634052 [details] [diff] [review]
Part 1 - Remove the dependency of XHR::send on a cx parameter. v2

Updating per above comments and try bustage.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-06-18 08:30:38 PDT
Created attachment 634053 [details] [diff] [review]
Part 2 - Remove js context parameter from XHR.send call chain. v2
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-06-18 08:45:05 PDT
Fix the XXXbz comment in Bindings.conf too?
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-06-18 08:53:34 PDT
Created attachment 634060 [details] [diff] [review]
Part 2 - Remove js context parameter from XHR.send call chain. v3

Updated per bz's request.
Comment 15 Olli Pettay [:smaug] (reviewing overload) 2012-06-19 10:28:42 PDT
Comment on attachment 634052 [details] [diff] [review]
Part 1 - Remove the dependency of XHR::send on a cx parameter. v2


>+    nsCxPusher pusher;
>     JSObject* obj;
>+    JSContext* cx = nsContentUtils::GetCurrentJSContext();
>+    if (!cx) {
>+      cx = nsContentUtils::GetSafeJSContext();
>+      pusher.Push(cx);
>+    }
This needs a comment why Push isn't needed if GetCurrentJSContext return non-null.
Also, please check the return value of Push


>     nsresult rv = aBody->GetAsJSVal(&realVal);
>     if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) &&
>         (obj = JSVAL_TO_OBJECT(realVal)) &&
>-        (JS_IsArrayBufferObject(obj, aCx))) {
>-      ArrayBuffer buf(aCx, obj);
>+        (JS_IsArrayBufferObject(obj, cx))) {
>+      ArrayBuffer buf(cx, obj);
>       return GetRequestBody(&buf, aResult, aContentType, aCharset);
>     }
Is this safe if obj and cx are from different compartments?
It feels like the change to use safecontext may increase the probability for compartment mismatch.
(I'd say the old API requires caller to ensure compartment handling.)
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-06-20 02:22:03 PDT
Created attachment 634821 [details] [diff] [review]
Part 1 - Remove the dependency of XHR::send on a cx parameter. v3

Addressed smaug's review comments. Reflagging.

Note You need to log in before you can comment on or make changes to this bug.