Closed Bug 1014993 Opened 8 years ago Closed 8 years ago

Make it easy to distinguish CPOWs from normal objects

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(2 files)

These patches should make it easier to debug code where CPOWs are involved.
Attached patch is-cpowSplinter Review
This patch adds Cu.isCrossProcessWrapper.
Attachment #8427456 - Flags: review?(mrbkap)
Attached patch cpow-tostringSplinter Review
This patch specializes toString for CPOWs. It avoids doing anything special if toString doesn't start with '['.
Attachment #8427457 - Flags: review?(mrbkap)
Keywords: dev-doc-needed
OS: Linux → All
Hardware: x86_64 → All
Attachment #8427456 - Flags: review?(mrbkap) → review+
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?
Attachment #8427457 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/8f40db8873da
https://hg.mozilla.org/mozilla-central/rev/c0f3fad10f91
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.)
(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.
You need to log in before you can comment on or make changes to this bug.