Note: There are a few cases of duplicates in user autocompletion which are being worked on.

XHR.send shouldn't take a jscontext

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
Blocks: 754202

Comment 1

5 years ago
It is used in GetRequestBody

Comment 2

5 years ago
Er, nm.
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.
Attachment #633472 - Flags: review?(bugs)
Created attachment 633473 [details] [diff] [review]
Part 2 - Remove js context parameter from XHR.send call chain. v1
Attachment #633473 - Flags: review?(bugs)

Comment 5

5 years ago
> 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

5 years ago
I don't think we use new bindings with createInstance, since in that case XHR isn't parented to
anything (GetParentObject() returns null).
(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

5 years ago
Oh, I thought in new bindings world wrappercached objects need to have some parent. Apparently no.

Comment 9

5 years ago
We changed that, for certain classes of use cases.
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.
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.
Attachment #633472 - Attachment is obsolete: true
Attachment #633472 - Flags: review?(bugs)
Attachment #634052 - Flags: review?(bugs)
Created attachment 634053 [details] [diff] [review]
Part 2 - Remove js context parameter from XHR.send call chain. v2
Attachment #633473 - Attachment is obsolete: true
Attachment #633473 - Flags: review?(bugs)
Attachment #634053 - Flags: review?(bugs)

Comment 13

5 years ago
Fix the XXXbz comment in Bindings.conf too?
Created attachment 634060 [details] [diff] [review]
Part 2 - Remove js context parameter from XHR.send call chain. v3

Updated per bz's request.
Attachment #634053 - Attachment is obsolete: true
Attachment #634053 - Flags: review?(bugs)
Attachment #634060 - Flags: review?(bugs)
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.)
Attachment #634052 - Flags: review?(bugs) → review-

Updated

5 years ago
Attachment #634060 - Flags: review?(bugs) → review+
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.
Attachment #634052 - Attachment is obsolete: true
Attachment #634821 - Flags: review?(bugs)

Updated

5 years ago
Attachment #634821 - Flags: review?(bugs) → review+
Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b344d7df7b1a
http://hg.mozilla.org/integration/mozilla-inbound/rev/9da89830c1ae
https://hg.mozilla.org/mozilla-central/rev/9da89830c1ae
https://hg.mozilla.org/mozilla-central/rev/b344d7df7b1a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.