Closed Bug 1344209 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: xidorn, Unassigned)

References

Details

Attachments

(1 file)

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.
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.
Priority: -- → P1
mystor, could you have a look at this?
Flags: needinfo?(michael)
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.
Also I'll add an " in rust nsstring bindings" to the commit message I guess :)
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 :).
Flags: needinfo?(michael)
Attachment #8843293 - Flags: review?(michael) → review+
Now with tests and a decent commit message.
Attachment #8843293 - Flags: review?(michael) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec25e01a8ff1
Handle empty strings gracefully in rust nsString bindings. r=mystor
https://hg.mozilla.org/mozilla-central/rev/ec25e01a8ff1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.