Closed Bug 1225923 Opened 4 years ago Closed 4 years ago

get rid of array.AppendElement(nsDependent*String(...)) calls

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(5 files, 1 obsolete file)

It's not especially obvious what these calls do: do they create a string in the
array that is also a dependent string or do they create a string in the array
which is a (allocated) copy of the temporary dependent string.  Presumably the
former was intended, but the latter is what we get.  And sometimes even when
the former is desired, having the latter happen saves us from possible
use-after-free errors.  Let's try to make that distinction explicit in the
code, rather than requiring somebody to be an expert in nsString code.
When people write:

  array.AppendElement(nsDependentString(...));

(resp. nsDependentCString), it's not clear whether they expect the newly
constructed dependent string to live in the array, or whether they're
just making a nsString-like holder whose contents can be freely copied
into the array's newly-created nsString.  Sometimes the latter is what
you prefer, and sometimes the former.  In all cases, however, the latter
behavior is what you get.

Let's try to make that behavior more explicit by pre-constructing
nsString elements and then using Assign to show that copying is taking
place.  This patch involves no functional change in behavior (it ought
to be epsilon faster due to using AppendElements, rather than repeatedly
calling AppendElement).
Attachment #8689100 - Flags: review?(erahm)
I think the intent of this call is to not copy the filename data passed
in, but to simply convert it to a friendlier nsString container for
processing.  We don't have to worry about the nsTArray<nsString>
overload of MozSetFileNameArray holding references to the strings
outside the call, as the elements of the array are copied into new
strings as appropriate.
Attachment #8689101 - Flags: review?(amarchesini)
I think the intent of these calls is to not copy the information strings
passed in, but to simply convert them to a friendlier nsString container
for processing.

In the TelephonyDialCallback case, we're copying the strings only to
convert them to an array of JS::Value, which copies the data again.  We
might as well avoid one of the copies.

In the TelephonyParent case, we're copying the strings only send them
over an IPC channel.  Therefore, it's safe to not copy the strings
initially, because the original strings will remain live long enough to
send the IPC message.
Attachment #8689102 - Flags: review?(btseng)
I think the intent of this call is not to copy the flavor data passed
in, but to simply convert it to a friendlier nsCString container for
serializing to an IPC message.  Since we won't be retaining references
to the passed-in strings after this function returns, we can make the
elements of our temporary array actually dependent strings, rather than
creating temporary dependent strings that would need to be copied into
the array.
Attachment #8689103 - Flags: review?(roc)
The intent here is clearly "add a string to this array that depends on
this atom".  Unfortunately, that's not what we do: we create a temporary
dependent string and then copy it into the array, destroying the point
of trying to make it depend on the atom in the first place.

We can't get rid of nsDependentAtomString; far too many places depend on
it.  What we can do is add a separate Rebind() call to nsString that
wraps the details of being dependent on an atom, and use that everywhere
we wanted to save memory by creating these strings.

I'm not a huge fan of giving nsString a dependency on nsIAtom, but doing that
seemed a little better than writing out the nsIAtom calls everywhere...
Attachment #8689104 - Flags: review?(bzbarsky)
Attachment #8689101 - Flags: review?(amarchesini) → review+
Comment on attachment 8689104 [details] [diff] [review]
part 5 - fix AppendElement(nsDependentAtomString(...)) calls

> The intent here is clearly "add a string to this array that depends on
> this atom".

No, it's not.  The intent for all the GetSupportedNames methods is "copy this atom's string into this array".  It has to be, because the atom may not outlive the array!

I used nsDependentAtomString there because it's the simplest/cheapest way to get an nsAString out of an atom that we can then append.

The other thing we could do is something like "atom->ToString(*array.AppendElement());", right?
Attachment #8689104 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8689104 [details] [diff] [review]
> part 5 - fix AppendElement(nsDependentAtomString(...)) calls
> 
> > The intent here is clearly "add a string to this array that depends on
> > this atom".
> 
> No, it's not.  The intent for all the GetSupportedNames methods is "copy
> this atom's string into this array".  It has to be, because the atom may not
> outlive the array!

Always a good sign when the patch author is confused by the code he's attempting to modify...

Isn't that the point of Atoms--that they're going to stick around for...a long time?

> The other thing we could do is something like
> "atom->ToString(*array.AppendElement());", right?

I like that idea better than adding nsIAtom bits into nsString.
Flags: needinfo?(bzbarsky)
> Isn't that the point of Atoms--that they're going to stick around for...a long time?

Sadly, no  The point is to make compares fast.  They're interned, not pinned in any way.  Static atoms will stick around for a while.  Non-static ones will go away as soon as their refcount goes to 0.

> I like that idea better than adding nsIAtom bits into nsString.

OK, let's do that.
Flags: needinfo?(bzbarsky)
Comment on attachment 8689100 [details] [diff] [review]
part 1 - convert all needs-to-copy instances of AppendElement(nsDependentString(...))

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

r=me. Looks like there might be one place where a dependent string is warranted, but seeing as how you haven't changed the actual behavior I don't feel to strongly about changing it.

::: storage/mozStorageStatement.cpp
@@ +152,3 @@
>    for (uint32_t i = 0; i < mResultColumnCount; i++) {
>        const char *name = ::sqlite3_column_name(mDBStatement, i);
> +      columnNames[i].Assign(name);

In this case I think they really did want dependent strings. The |name| returned from |sqlite3_column_name| is a reference to an internal string that is valid until finalized [1].

There is a bit that's not super clear, "or until the statement is automatically reprepared by the first call to sqlite3_step()," so I guess we could be paranoid and leave this as you have it.

[1] https://www.sqlite.org/c3ref/column_name.html
Attachment #8689100 - Flags: review?(erahm) → review+
Attachment #8689104 - Attachment is obsolete: true
(In reply to Eric Rahm [:erahm] from comment #9)
> There is a bit that's not super clear, "or until the statement is
> automatically reprepared by the first call to sqlite3_step()," so I guess we
> could be paranoid and leave this as you have it.

Yeah, I'm not entirely sure if it's worth it to try to be clever in parts 2-4 because of things like this...
Comment on attachment 8689155 [details] [diff] [review]
part 5 - fix AppendElement(nsDependentAtomString(...)) calls

r=me
Attachment #8689155 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689102 [details] [diff] [review]
part 3 - fix AppendElement(nsDependentString(...)) calls in dom/telephony/

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

LGTM, thank you!
Attachment #8689102 - Flags: review?(btseng) → review+
You need to log in before you can comment on or make changes to this bug.