Closed Bug 1377351 Opened 7 years ago Closed 7 years ago

Consider adding a move constructor and move assign overload/method to nsA[C]String

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(5 files, 5 obsolete files)

3.02 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.71 KB, patch
Details | Diff | Splinter Review
8.55 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
7.69 KB, patch
froydnj
: feedback+
Details | Diff | Splinter Review
8.67 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
I have opinions as to what this should act like.

I think that we should attempt to move the buffer in the case that the assigned-to string will not have dependent strings, or strings which don't match the expected semantics of the receiving string type. Namely I imagine that the following should happen:

1. If F_TERMINATED is not set on the source string, perform a copy, and truncate the source string.
2. If F_SHARED, F_OWNED, or F_LITERAL is set on the source string, transfer ownership (or the reference to in the case of F_LITERAL) of the backing buffer to the target string, and truncate the source string.
3. For all other strings, perform a copy, and truncate the source string.

This ensures that the target string always has a reference to string data which should live perpetually, and the source string is always truncated, while performing no copies in the situations where we can get away with it. I like this as it provides very predictable behavior which is also performant.

I imagine it would be very confusing and dangerous to copy dependent string buffer references when using a move operator, and we definitely don't want to share a reference into a fixed string buffer, as it will be likely to go away or mutate.
Assignee: nobody → michael
Comment on attachment 8908186 [details] [diff] [review]
Part 1: Add move overload to nsA[C]String::Assign

Review of attachment 8908186 [details] [diff] [review]:
-----------------------------------------------------------------

These patches need some tests, preferably testing a couple different variations: {owned, shared, auto, etc} x {owned, shared, auto, etc}.  I'm not completely sure how to test some things, since we might have static analyses that ensure that you don't touch moved-from things after moving from them?

I guess there would be some testing from people returning ns*String from functions, which would automatically start using move constructors, so maybe UAF and leaks would get checked-for by ASan etc.  Regardless, having standalone tests for this sort of stuff would be good to make sure that we really understand the entire space here.
Attachment #8908186 - Flags: review?(nfroyd)
Comment on attachment 8908187 [details] [diff] [review]
Part 2: Add move overloads to nsA[C]String assignment and constructors

Review of attachment 8908187 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing for tests from comment 1.
Attachment #8908187 - Flags: review?(nfroyd)
Comment on attachment 8908188 [details] [diff] [review]
Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust

Review of attachment 8908188 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/rust/nsstring/src/lib.rs
@@ +304,5 @@
> +                unsafe { $take_from(self, other) };
> +            }
> +
> +            /// Take the value of `other` and set `self`, overwriting any value
> +            /// currently stored. The passed-in string will be truncated.

This is the ideal state of things if we return Ok(); what happens if Err() is returned?
Attachment #8908188 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #6)
> ::: xpcom/rust/nsstring/src/lib.rs
> @@ +304,5 @@
> > +                unsafe { $take_from(self, other) };
> > +            }
> > +
> > +            /// Take the value of `other` and set `self`, overwriting any value
> > +            /// currently stored. The passed-in string will be truncated.
> 
> This is the ideal state of things if we return Ok(); what happens if Err()
> is returned?

The code currently unconditionally truncates the source string unless &source == &dest, so if Err() is returned then the assignment failed but the source string was destroyed. Perhaps we want to not destroy the source string in that situation.
This test passes locally with debug builds for me. I think it tests the interesting cases decently well.

In addition, I'm fairly confident that the logic in ::Assign(nsA[C]String&&) is
correct, as there's only one branch (source is either OWNED or SHARED) which is
different than the normal Assign path. All other types perform a normal
copy-assign followed by a `Truncate`.
Attachment #8908318 - Flags: review?(nfroyd)
Attachment #8908186 - Flags: review?(nfroyd)
Attachment #8908187 - Flags: review?(nfroyd)
Attachment #8908188 - Flags: review?(nfroyd)
Comment on attachment 8908318 [details] [diff] [review]
Part 4: Add tests for nsA[C]String::Assign move overload

Review of attachment 8908318 [details] [diff] [review]:
-----------------------------------------------------------------

These are probably not bad tests for all manner of string functionality, not just move operations.

We should have some tests for moving from actual substrings (probably nsDependent*Substring) to ensure that the non-TERMINATED case works correctly.  (It should, since substrings should not be SHARED or OWNED, but "trust, but verify".)

::: xpcom/tests/gtest/TestMoveString.cpp
@@ +31,5 @@
> +  EXPECT_EQ(aStr.GetDataFlags(), Df::OWNED | Df::TERMINATED);
> +  EXPECT_STREQ(aStr.BeginReading(), aValue);
> +}
> +
> +void ExpectTerminated(const nsACString& aStr)

Maybe this should be ExpectEmptyString?  I guess by "Terminated" you were going for the "dead" meaning rather than the ns*String "ends in a null" meaning?
Attachment #8908318 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8908186 [details] [diff] [review]
Part 1: Add move overload to nsA[C]String::Assign

Review of attachment 8908186 [details] [diff] [review]:
-----------------------------------------------------------------

I feel better about this patch now that we're going to have tests.
Attachment #8908186 - Flags: review?(nfroyd) → review+
Attachment #8908187 - Flags: review?(nfroyd) → review+
Comment on attachment 8908188 [details] [diff] [review]
Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust

Review of attachment 8908188 [details] [diff] [review]:
-----------------------------------------------------------------

I think we want to try and not destroy the source string here; can you see if that behavior is reasonable, and if so, add tests for it (Rust or C, your choice)?
Attachment #8908188 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #9)
> These are probably not bad tests for all manner of string functionality, not
> just move operations.

Are you suggesting I copy these tests and write a non-moving version of them?

> We should have some tests for moving from actual substrings (probably
> nsDependent*Substring) to ensure that the non-TERMINATED case works
> correctly.  (It should, since substrings should not be SHARED or OWNED, but
> "trust, but verify".)

The nsDependentCSubstring method does try to use an actual substring (I explicitly use the char*/length constructor for nsDependentCSubstring, and confirm that the source doesn't have the TERMINATED flag set). The source string _does_ actually have a terminator though - I'll try adding some garbage on the end to make sure it works well.

> ::: xpcom/tests/gtest/TestMoveString.cpp
> @@ +31,5 @@
> > +  EXPECT_EQ(aStr.GetDataFlags(), Df::OWNED | Df::TERMINATED);
> > +  EXPECT_STREQ(aStr.BeginReading(), aValue);
> > +}
> > +
> > +void ExpectTerminated(const nsACString& aStr)
> 
> Maybe this should be ExpectEmptyString?  I guess by "Terminated" you were
> going for the "dead" meaning rather than the ns*String "ends in a null"
> meaning?

I meant to write ExpectTruncated, and apparently just... wrote something completely different? - (I wanted to confirm that the string was in the same state that it would be after calling Truncate()).

I'm going to stick with that name unless you prefer ExpectEmptyString.

(In reply to Nathan Froyd [:froydnj] from comment #11)
> I think we want to try and not destroy the source string here; can you see
> if that behavior is reasonable, and if so, add tests for it (Rust or C, your
> choice)?

I'm not sure how to cause the string allocation to fail to test this behaviour :-/. Do we have a way to force allocations to fail for testing purposes?
GCC 4.9 has some problems resolving expressions like the following when adding a
new operator=(T&&) overload to nsTSubstring:
aOutAString = NS_LITERAL_STRING("...");

This appears to be due to a frontend bug which means that adding another
operator= overload causes some coersions to not occur implicitly. Namely the
expression above ends up generating implicit calls like:

(nsTSubstring& aOutAString)::operator=(
  nsTSubstring&& rvalue_ref_temporary(
    nsTSubstring nsTSubstring( // << This constructor is protected
      const nsTSubstring& derive_to_base(
        operator const nsTString&(
          const nsTLiteralString& ...)))))

Which causes a failure, as the nsTSubstring copy constructor is protected, and
we could instead use a different overload of operator= (which is used in more
recent versions of gcc):

(nsTSubstring& aOutAString)::operator=(
  const nsTSubstring& derive_to_base(
    operator const nsTString&(
      const nsTLiteralString& ...)))

This change means that the implicit cast operator is no longer necessary, and we
can use the following coercion which doesn't mess up on gcc 4.9:

(nsTSubstring& aOutAString)::operator=(
  const nsTSubstring& derive_to_base(
      const nsTLiteralString& ...))
Attachment #8911940 - Flags: review?(nfroyd)
I changed the behaviour here to match what you asked for in comment 11.
Attachment #8912279 - Flags: review?(nfroyd)
Attachment #8908186 - Attachment is obsolete: true
Attachment #8908187 - Attachment is obsolete: true
Attachment #8908188 - Attachment is obsolete: true
Attachment #8908318 - Attachment is obsolete: true
I changed the docs here to match the change in part 1
Attachment #8912281 - Flags: review?(nfroyd)
I changed the tests a bit like you asked, but I'm not sure if
a) You want me to generalize these tests to also test non-moving assignments, and
b) How to test allocation failure cases.

I'd love some input on how to test those, so I'm requesting feedback.
Attachment #8912282 - Flags: feedback?(nfroyd)
Comment on attachment 8912279 [details] [diff] [review]
Part 1: Add move overload to nsA[C]String::Assign

Review of attachment 8912279 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8912279 - Flags: review?(nfroyd) → review+
Comment on attachment 8912282 [details] [diff] [review]
Part 4: Add tests for nsA[C]String::Assign move overload

Review of attachment 8912282 [details] [diff] [review]:
-----------------------------------------------------------------

Testing just the moving assignments is OK, thanks.

Allocation failure cases are hard.  We don't really have anything good for that on the Gecko side of things besides "allocate something really big on 32-bit platforms". =/  If ns*String took an AllocPolicy parameter, our tests could sub in an OOM-simulating one, but that's a pretty big job...
Attachment #8912282 - Flags: feedback?(nfroyd) → feedback+
Comment on attachment 8911940 [details] [diff] [review]
Part 5: Make nsTLiteralString inherit from nsTSubstring instead of nsTStringRepr

Review of attachment 8911940 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not super excited about this patch, since this just gets us back to the pre-bug 1344629 state of affairs, and looks like it reintroduces some static constructors while doing so.  Could we just alter the (ideally few) places that are using assignment from literal string temporaries to use aOutAString.AssignLiteral(...), which is what they should have been using anyway?

::: xpcom/string/nsTLiteralString.h
@@ -17,5 @@
>   * nsTString-lookalike that restricts its string value to a literal character
>   * sequence. Can be implicitly cast to const nsTString& (the const is
>   * essential, since this class's data are not writable). The data are assumed
>   * to be static (permanent) and therefore, as an optimization, this class
>   * does not have a destructor.

This comment would no longer be true if this patch lands.
Attachment #8911940 - Flags: review?(nfroyd) → review-
Comment on attachment 8912281 [details] [diff] [review]
Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust

Review of attachment 8912281 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/rust/nsstring/src/lib.rs
@@ +305,5 @@
> +            }
> +
> +            /// Take the value of `other` and set `self`, overwriting any value
> +            /// currently stored. If this function fails, the source string will
> +            /// be left untouched, otherwise it will be truncated.

This comment seems confusing to me: hiding the truncation-on-success at the end of the second sentence reads strangely.  Maybe:

Take the value of `other` and set `self`, overwriting any value currently stored.  The source string will be truncated on success.  If this function fails (e.g. a copy of the source string can not be made), the source string will be left untouched.
Attachment #8912281 - Flags: review?(nfroyd) → review+
Also note that I was looking at the static constructor numbers on Linux, whereas the motivating example for bug 1344629 was static constructors on Windows.  So the situation from p5 on Windows might actually be worse.
Depends on: 1404040
This is going to be a pain to fix in a non-undefined-behaviour-filled way with GCC 4.9, so I'm inclined to leave it until then.
I had some time while waiting for try pushes to try this out again, and I think I got a workaround which works.

MozReview-Commit-ID: CHywpZ4fvXf
Attachment #8915749 - Flags: review?(nfroyd)
Attachment #8911940 - Attachment is obsolete: true
Comment on attachment 8915749 [details] [diff] [review]
Part 5: Add a workaround for gcc 4.9 compiler bug

Review of attachment 8915749 [details] [diff] [review]:
-----------------------------------------------------------------

This works for me, and is less terrible than previous versions.

::: xpcom/string/nsTString.h
@@ +126,5 @@
>    }
>  
> +  // NOTE(nika): gcc 4.9 workaround. Remove when support is dropped.
> +  explicit
> +  nsTString(const literalstring_type& aReadable)

Nit: This is sort of peculiar formatting; I think the usual would be putting this all on one line:

explicit nsTString(const literalstring_type& aReadable)

@@ +663,5 @@
>    }
>  
> +  // NOTE(nika): gcc 4.9 workaround. Remove when support is dropped.
> +  explicit
> +  nsTAutoStringN(const literalstring_type& aStr)

Same nit with the formatting here.
Attachment #8915749 - Flags: review?(nfroyd) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf17a82ef4a9
Part 1: Add move overload to nsA[C]String::Assign, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8f494ccca3
Part 2: Add move overloads to nsA[C]String assignment and constructors, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f081c828265
Part 3: Expose nsA[C]String::Assign(nsA[C]String&&) overload as take_from to rust, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa544eccfc2
Part 4: Add tests for nsA[C]String::Assign move overload, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/6206e82fc670
Part 5: Add a workaround for gcc 4.9 compiler bug, r=froydnj
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: