1135731 - inconsistent encoding in NS_NewXBLProtoImpl

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Tom Tromey :tromey]

In the review for bug 987069, Waldo asked me to file this bug.

NS_NewXBLProtoImpl has an encoding inconsistency:

  if (aClassName)
    impl->mClassName.AssignWithConversion(aClassName);
  else
    aBinding->BindingURI()->GetSpec(impl->mClassName);

GetSpec returns a UTF-8 encoded string, but AssignWithConversion converts
to Latin-1 -- it says ASCII but that is not true, in that it does not
strip the high bit from characters > 127.


Simply changing this to use NS_ConvertUTF16toUTF8 is a good start.
However I think it runs into some other issues.

nsXBLProtoImpl::InitTargetObjects calls 

  rv = aBinding->InitClass(mClassName, cx, value, aTargetClassObject, aTargetIsNew);

which winds up in a call to Atomize, which expects Latin-1.
(Via nsXBLBinding::DoInitJSClass -> JS_GetOwnPropertyDescriptor)

There's also Write:

  rv = aStream->WriteStringZ(mClassName.get());

which should probably be writeStringUtf8Z.

There are also callers of nsXBLPrototypeBinding::ClassName to contend with.

I'm thinking perhaps changing mClassName to be an nsACString is the safest.
I haven't tried this yet, so perhaps there are hidden gotchas.

Comment 1

[Tom Tromey :tromey]

There's also a minor discrepancy in nsXBLPrototypeBinding::Write.
It doesn't affect the results at all, but it is confusing if one
is reading the source closely.  Right now it does:

  // Write out the implementation details.
  if (mImplementation) {
    rv = mImplementation->Write(aStream, this);
    NS_ENSURE_SUCCESS(rv, rv);
  }
  else {
    // Write out an empty classname. This indicates that the binding does not
    // define an implementation.
    rv = aStream->WriteWStringZ(EmptyString().get());
    NS_ENSURE_SUCCESS(rv, rv);
  }

but mImplementation->Write uses WriteStringZ, not WriteWStringZ.

Comment 2

[Tom Tromey :tromey]

> There's also Write:
> 
>   rv = aStream->WriteStringZ(mClassName.get());
> 
> which should probably be writeStringUtf8Z.

This is incorrect, even discounting the spelling mistake,
since WriteUtf8Z takes a wstring.  The current code here
is sensible assuming UTF-8 input.

Comment 3

[Tom Tromey :tromey]

Attachment: fix encoding inconsistency in NS_NewXBLProtoImpl

This changes mClassName to be an nsString and fixes up the fallout.  I
chose this approach because it's simpler to prove that the resulting
encodings are consistent and correct.  Using UTF-8 would be possible,
but more troublesome, due to things like Atomize accepting Latin-1.

One concern I had with this is whether space is an issue here.
If so then I can try harder with the UTF-8 approach.

Comment 4

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fb068aae20e

Comment 5

[Tom Tromey :tromey]

I tried and I could not figure out a way to test this.
I think the test case is to make an <implementation name="non-latin-1-stuff">
and then examine the resulting name from javascript.
However, I couldn't see how to do this.  Any hint would be appreciated.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I've been looking at this for the past couple of days now and I don't see how to test this either. Sorry for taking so long to get back to you with a useless answer :(

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8568074 [details] [diff] [review]
fix encoding inconsistency in NS_NewXBLProtoImpl

I think this is ready for review -- unless someone has another
suggestion for how to test it.
Also requesting review from :Waldo for the JS changes.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8568074 [details] [diff] [review]
fix encoding inconsistency in NS_NewXBLProtoImpl

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

There are a few small nits to pick. r=me with them addressed.

::: dom/xbl/nsXBLProtoImpl.cpp
@@ +525,2 @@
>    else
> +    {

Nits: if the else requires braces, we add them to the if clause as well. We also follow (mostly) K&R-style, so:

if (...) {
} else {
}

instead of GNU-style.

::: dom/xbl/nsXBLProtoImplMethod.cpp
@@ +188,5 @@
>  
>    // Now that we have a body and args, compile the function
>    // and then define it.
>    NS_ConvertUTF16toUTF8 cname(mName);
> +  nsAutoCString functionUri = NS_ConvertUTF16toUTF8(aClassStr);

This could just be:

NS_ConvertUTF16toUTF8 functionUri(aClassStr);

right?

::: dom/xbl/nsXBLPrototypeBinding.h
@@ +94,3 @@
>    }
>  
> +  nsresult InitClass(const nsString& aClassName, JSContext * aContext,

Nit (pre-existing): Might as well move the * next to JSContext for consistency.

Comment 9

[Tom Tromey :tromey]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #8)

> Nits: if the else requires braces, we add them to the if clause as well. We
> also follow (mostly) K&R-style, so:

> instead of GNU-style.

I'm usually aware of it, but this one slipped right by me.  Old habits I guess.

> > +  nsAutoCString functionUri = NS_ConvertUTF16toUTF8(aClassStr);
> 
> This could just be:
> 
> NS_ConvertUTF16toUTF8 functionUri(aClassStr);
> 
> right?

Yes indeed.

Thanks for the review, uploading the new one momentarily.

Comment 10

[Tom Tromey :tromey]

Attachment: fix encoding inconsistency in NS_NewXBLProtoImpl

Comment 11

[Tom Tromey :tromey]

I was not sure if this patch required additional review for the spidermonkey
changes, so NI'ing the both of you.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

My review is also good for the SpiderMonkey changes.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Recent try run, please.

Comment 14

[Tom Tromey :tromey]

Attachment: fix encoding inconsistency in NS_NewXBLProtoImpl

Rebased.

Comment 15

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05cf18f24d45

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4777149382bc

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4777149382bc

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4777149382bc