Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Description • 10 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 1 • 10 years ago

This patch adds Cu.isCrossProcessWrapper.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 2 • 10 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 3 • 10 years ago

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?

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 4 • 10 years ago

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

	Tim Taubert [:ttaubert] (inactive)
	Comment 5 • 10 years ago

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

	neil@parkwaycc.co.uk
	Comment 6 • 10 years ago

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.)

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 7 • 10 years ago

(In reply to neil@parkwaycc.co.uk 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.