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.

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.

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

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.

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.

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

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

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

My review is also good for the SpiderMonkey changes.

Recent try run, please.

Rebased.