1344209 - nsstring binding crashes when calling nsCString::from with an empty String

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Xidorn Quan [:xidorn] UTC+10]

So I believe the following code destroys everything:
> let s = String::new();
> let x = nsCString::from(s);

This is because
> String::new().into_bytes().into_boxed_slice().as_ptr()
returns 0x1 rather than a valid pointer, but nsstring assumes it is a valid pointer.

According to IRC chat, Ms2ger said that is the expected behavior in Rust, and nsstring binding should not assume the pointer is valid.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

This issue triggers crash in stylo when we start using empty string for initial value of animation-name, so it should be relatively a high priority issue, because it now blocks our three largest tests.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

mystor, could you have a look at this?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1344209: Handle empty strings gracefully in rust nsString bindings.

Review commit: https://reviewboard.mozilla.org/r/117090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117090/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I'm not sure how to run the tests for this. I don't have the time to investigate with our rust + gtest setup right now, but I'll add a test before landing if Michael gives me a hint.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Also I'll add an " in rust nsstring bindings" to the commit message I guess :)

Comment 6

[Nika Layzell [:nika] (ooo, ni? for response)]

Comment on attachment 8843293 [details]
Bug 1344209: Handle empty strings gracefully in rust nsString bindings.

R=me. To run tests do a normal build then run mach gtest RustNsString.*. You have to add the tests in the C++ file. 

Hope that helps :). I'd like to see the test before you land it :).

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8843293 [details]
Bug 1344209: Handle empty strings gracefully in rust nsString bindings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117090/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Now with tests and a decent commit message.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec25e01a8ff1
Handle empty strings gracefully in rust nsString bindings. r=mystor

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/ec25e01a8ff1