1014993 - Make it easy to distinguish CPOWs from normal objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

These patches should make it easier to debug code where CPOWs are involved.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: is-cpow

This patch adds Cu.isCrossProcessWrapper.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: cpow-tostring

This patch specializes toString for CPOWs. It avoids doing anything special if toString doesn't start with '['.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8427457 [details] [diff] [review]
cpow-tostring

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

r=me with my comments addressed.

::: js/ipc/WrapperOwner.cpp
@@ +330,5 @@
> +
> +bool
> +WrapperOwner::toString(JSContext *cx, HandleObject cpow, JS::CallArgs &args)
> +{
> +    static const char start[] = "[CPOW ";

I'd prefer that this be "[object CPOW " to match Xrays and Objects.

@@ +351,5 @@
> +    // We don't want to wrap toString() results for things like the location
> +    // object, where toString() is supposed to return a URL and nothing else.
> +    nsAutoString result;
> +    if (chars[0] == '[') {
> +        result.AppendASCII(start);

You can probably use AppendLiteral here, right?

@@ +352,5 @@
> +    // object, where toString() is supposed to return a URL and nothing else.
> +    nsAutoString result;
> +    if (chars[0] == '[') {
> +        result.AppendASCII(start);
> +        result += nsString(chars);

Instead of nsString which will copy chars, can you use an nsDependentJSString here and below?

@@ +397,5 @@
> +            return false;
> +
> +        RootedObject toStringObj(cx, JS_GetFunctionObject(toString));
> +
> +        if (!JS_DefineProperty(cx, toStringObj, "__cpow__", vp, 0))

Maybe JSPROP_PERMANENT/JSPROP_READONLY to reduce the chances of someone shooting themselves in the foot?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8f40db8873da
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f3fad10f91

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8f40db8873da
https://hg.mozilla.org/mozilla-central/rev/c0f3fad10f91

Comment 6

[[e-mail]]

Comment on attachment 8427457 [details] [diff] [review]
cpow-tostring

>+    if (chars[0] == '[') {
If the string doesn't begin with a '[', why do you have to do anything here? (Also, why is it so hard to find the first character of a JS string? All the APIs I can find want to make the string linear, if not flat.)

>+    static const char start[] = "[CPOW ";
>+    static const char end[] = "]";
...
>+        result.AppendASCII(start);
>+        result += nsString(chars);
>+        result.AppendASCII(end);
Append(']') is slightly nicer than AppendLiteral("]"), but technically the best possible code is
result = NS_LITERAL_STRING("[object CPOW ") + nsDependentString(chars) + NS_LITERAL_STRING("]");
as this computes the total length up front (it's possible that the appending of the trailing ']' causes a reallocation...)

>+        result += nsString(chars);
This was just terrible code, as you were copying three times:
* From chars into the nsString temporary object
* From the nsString temporary into result
* From result into str
(Using nsDependentJSString (which itself is horrible code) is one of the ways to save one copy; result = nsString(chars); or result.Append(chars); would also have worked, but you can't combine those methods to save more than one copy.)

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to [e-mail] from comment #6)
> This was just terrible code, as you were copying three times:

I haven't used nsString before, so this is helpful feedback. But please try to be more positive next time.