I have some patches to add some assertions to the string classes about null-termination invariants. More details coming in the patch descriptions...

This is needed for patch 4. MozReview-Commit-ID: 5ikQFIL9O0i

This is needed for patch 4. MozReview-Commit-ID: 7xKQv37cy5k

This is needed for patch 4. MozReview-Commit-ID: 4BFlTtQdtoN

I actually couldn't figure out a way to trigger this assertion with the current string code without doing invalid casts, but there are things we may want to add to the string code in the future that might risk hitting this (e.g., move constructors, promoting various rebind methods to nsA[C]String), so I think it's worth asserting. MozReview-Commit-ID: 4R0dYuTfrFW

This avoids triggering the assertions added in patch 6. MozReview-Commit-ID: 9000zY0FDId

While it would be nice to assert this in AssertValid() (added in the previous patch), this is difficult because of what MutatePrep does: https://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/xpcom/string/nsTSubstring.cpp#152,165,177-178 MozReview-Commit-ID: 12sQLHtJ4xc

I decided I don't actually want to land the last two patches, given how awkward it is to use the string API to do what the one caller that needed fixing wants to do. I think we should: * first, make that easier, and then * second, try to do the stronger assertions that I mention in patch 6 (i.e., in the same place as the code in patch 4)

Comment on attachment 8886693 [details] [diff] [review] patch 1 - Add ClassFlags::NULL_TERMINATED to strings that require null-termination Review of attachment 8886693 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/string/nsTString.h @@ +466,5 @@ > // allow subclasses to initialize fields directly > nsTString_CharT(char_type* aData, size_type aLength, DataFlags aDataFlags, > ClassFlags aClassFlags) > + : substring_type(aData, aLength, aDataFlags, > + aClassFlags | ClassFlags::NULL_TERMINATED) This one seems a little sketchy, it would be nice if there were a way to make sure we don't goof up and make a subclass that expects to *not* be null terminated. I'd almost prefer pushing the flag to the subclass and asserting they set NULL_TERMINATED, but I can see how that would require a lot more changes.

Comment on attachment 8886695 [details] [diff] [review] patch 3 - Encapsulate setting mData/mLength/mDataFlags in a new method Review of attachment 8886695 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/string/nsTDependentString.cpp @@ +27,5 @@ > startPos = strLength; > } > > + char_type* newData = > + const_cast<char_type*>(static_cast<const char_type*>(str.Data())) + startPos; Just curious, why do we need the static_cast? char16ptr_t weirdness? It would be nice to drop it if we don't need it anymore.

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #9) > This one seems a little sketchy, it would be nice if there were a way to > make sure we don't goof up and make a subclass that expects to *not* be null > terminated. Any subclass that isn't null-terminated should derive from nsTSubstring rather than nsTString. That's the key static type distinction between the two. (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #10) > Just curious, why do we need the static_cast? char16ptr_t weirdness? It > would be nice to drop it if we don't need it anymore. I noticed that too, and concluded from https://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/mfbt/Char16.h#20-21 that we probably still needed it.

Comment on attachment 8886696 [details] [diff] [review] patch 4 - Assert that strings whose static type requires a null-terminated buffer aren't assign a non-null-terminated buffer Review of attachment 8886696 [details] [diff] [review]: ----------------------------------------------------------------- This seems okay, a few notes below. ::: xpcom/string/nsTSubstring.h @@ -304,5 @@ > > protected: > nsTStringRepr_CharT() = delete; // Never instantiate directly > > - constexpr This could still be constexpr in release builds right? @@ +1093,5 @@ > { > mData = char_traits::sEmptyBuffer; > mLength = 0; > mDataFlags = DataFlags::TERMINATED; > + AssertValid(); Given the assertion we know this is always true right?

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #12) > > - constexpr > > This could still be constexpr in release builds right? I guess it could. Is there a reason it's valuable for it to be constexpr? > > mData = char_traits::sEmptyBuffer; > > mLength = 0; > > mDataFlags = DataFlags::TERMINATED; > > + AssertValid(); > > Given the assertion we know this is always true right? Yeah, but I decided I wanted it there anyway for future additions to AssertValid().

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #11) > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > #9) > > This one seems a little sketchy, it would be nice if there were a way to > > make sure we don't goof up and make a subclass that expects to *not* be null > > terminated. > > Any subclass that isn't null-terminated should derive from nsTSubstring > rather than nsTString. That's the key static type distinction between the > two. Yeah I understand, but my thought was it would be nice if we could statically enforce / assert that fact to avoid future issues. I guess that can just be "you need to understand this stuff" and code review from string person should catch it. > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > #10) > > Just curious, why do we need the static_cast? char16ptr_t weirdness? It > > would be nice to drop it if we don't need it anymore. > > I noticed that too, and concluded from > https://searchfox.org/mozilla-central/rev/ > cbd628b085ac809bf5a536109e6288aa91cbdff0/mfbt/Char16.h#20-21 that we > probably still needed it. I was thinking the implicit conversion to |const char16_t*| [1] might handle it for us, but maybe not inside a const_cast? [1] https://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/mfbt/Char16.h#51-54

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #13) > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > #12) > > > - constexpr > > > > This could still be constexpr in release builds right? > > I guess it could. Is there a reason it's valuable for it to be constexpr? David, you added a constexpr ctor in your nsTStringRepr class patch [1], is their a compelling reason to keep it constexpr? My guess is yes because (vaguely) it gives the compiler a chance to do some optimizations, but it would be nice to hear your reasoning. [1] https://reviewboard.mozilla.org/r/119210/diff/3#index_header

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #15) > David, you added a constexpr ctor in your nsTStringRepr class patch [1], is > their a compelling reason to keep it constexpr? My guess is yes because > (vaguely) it gives the compiler a chance to do some optimizations, but it > would be nice to hear your reasoning. > > [1] https://reviewboard.mozilla.org/r/119210/diff/3#index_header My first thought is that I must have added it to be extra-super-duper sure that global nsTLiteralStrings can be set up as just plain static data, without having to run constructors. But apparently I didn't add a constexpr constructor on nsTLiteralString itself, so I guess it's not a concern. My second guess would be that I added it as a safety net so that we error out if you add a new field and accidentally omit it from the ctor, since constexpr requires all members to be initialized.

...and/or possibly to reinforce the fact that nsTStringRepr is (was) just a dumb POD with no logic.

OK, maybe it's best to move the AssertValid() into nsTString (and just skip adding an equivalent to nsTLiteralString), and post a separate patch to add the constexpr to the nsTLiteralString constructor?

I actually couldn't figure out a way to trigger this assertion with the current string code without doing invalid casts, but there are things we may want to add to the string code in the future that might risk hitting this (e.g., move constructors, promoting various rebind methods to nsA[C]String), so I think it's worth asserting. MozReview-Commit-ID: 4R0dYuTfrFW

MozReview-Commit-ID: 5ASqQAGBqq

https://hg.mozilla.org/integration/mozilla-inbound/rev/79bc740e42aaeb256202eef44335a563594a7b04 Bug 1381080 patch 1 - Add ClassFlags::NULL_TERMINATED to strings that require null-termination. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e88978e73540cdd60f7a0374ac74b8fcfe0925 Bug 1381080 patch 2 - Encapsulate setting to empty buffer in a new method. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/235ac09dfdb2412295ac98834fb111e9198b0474 Bug 1381080 patch 3 - Encapsulate setting mData/mLength/mDataFlags in a new method. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/9b76213c99cfcb05a53d381edd272cbc29e6fadf Bug 1381080 patch 4 - Assert that strings whose static type requires a null-terminated buffer aren't assign a non-null-terminated buffer. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/5a663e876c3c2d76703368581b033f28495c10e1 Bug 1381080 patch 7 - Mark nsLiteral[C]String constructor as constexpr. r=erahm

https://hg.mozilla.org/mozilla-central/rev/79bc740e42aa https://hg.mozilla.org/mozilla-central/rev/e5e88978e735 https://hg.mozilla.org/mozilla-central/rev/235ac09dfdb2 https://hg.mozilla.org/mozilla-central/rev/9b76213c99cf https://hg.mozilla.org/mozilla-central/rev/5a663e876c3c