Closed Bug 1135731 Opened 5 years ago Closed 5 years ago

inconsistent encoding in NS_NewXBLProtoImpl

Categories

(Core :: XBL, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → ttromey
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.
Flags: needinfo?(mrbkap)
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 :(
Flags: needinfo?(mrbkap)
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.
Attachment #8568074 - Flags: review?(mrbkap)
Attachment #8568074 - Flags: review?(jwalden+bmo)
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.
Attachment #8568074 - Flags: review?(mrbkap) → review+
(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.
Attachment #8568074 - Attachment is obsolete: true
Attachment #8568074 - Flags: review?(jwalden+bmo)
I was not sure if this patch required additional review for the spidermonkey
changes, so NI'ing the both of you.
Flags: needinfo?(mrbkap)
Flags: needinfo?(jwalden+bmo)
My review is also good for the SpiderMonkey changes.
Flags: needinfo?(mrbkap)
Keywords: checkin-needed
Recent try run, please.
Keywords: checkin-needed
Rebased.
Attachment #8575999 - Attachment is obsolete: true
Flags: needinfo?(jwalden+bmo)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4777149382bc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.