use UniquePtr for storage in nsTreeContentView

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8728498 [details] [diff] [review]
use UniquePtr for storage in nsTreeContentView

This is a sort-of-straightforward search-and-replace.  The only wrinkle is that
where the previous code did:

  Row* row = mRows[...];

or similar, relying on the implicit conversion to Row*, we sometimes do:

  UniquePtr<Row>& row = mRows[...];

and sometimes we do:

  Row* row = mRows[...].get();

The latter is necessary because subsequent code in those functions can modify
|mRows| and thus invalidate our UniquePtr&.  I can convert everything to use
the latter form, if you like.
Attachment #8728498 - Flags: review?(dholbert)
(In reply to Nathan Froyd [:froydnj] from comment #1)
> The latter is necessary because subsequent code in those functions can modify
> |mRows| and thus invalidate our UniquePtr&.

Ooh, tricky.

(Looks like the latter is sometimes necessary for a simpler reason -- just because we reassign |row|, e.g. in HasNextSibling and GetLevel.)

> I can convert everything to use the latter form, if you like.

I think I would prefer that, yeah.  That seems like a safer conversion, in terms of likelihood-of-introducing-new-bugs.  And it reduces the amount of auditing that I'd feel that I need to do here (and that anyone would need to know to do, in the future, whenever changing this code), to be sure that absolutely nothing modifies the array (including in helper functions) after we've declared a local UniquePtr<Row>& variable.
Comment on attachment 8728498 [details] [diff] [review]
use UniquePtr for storage in nsTreeContentView

Canceling review for now -- assuming you're OK with the update per end of comment 1, I'll just review the updated patch.
Attachment #8728498 - Flags: review?(dholbert)
(Assignee)

Comment 4

3 years ago
Created attachment 8728572 [details] [diff] [review]
use UniquePtr for storage in nsTreeContentView
Attachment #8728572 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8728498 - Attachment is obsolete: true
Comment on attachment 8728572 [details] [diff] [review]
use UniquePtr for storage in nsTreeContentView

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

r=me, nits below:

::: layout/xul/tree/nsTreeContentView.cpp
@@ +1061,5 @@
>                              nsGkAtoms::_true, eCaseMatters))
>      return;
>  
> +  UniquePtr<Row>& row =
> +    *aRows.AppendElement(MakeUnique<Row>(aContent, aParentIndex));

I'd prefer:
  aRows.AppendElement(MakeUnique<Row>(aContent, aParentIndex));
  Row* row = aRows.LastElement().get();
...since I'm uncomfortable dealing with a reference to an object that lives in an array that could be reallocated depending on what we do with it.

(Also, the fact that we're using UniquePtr<>& as our local-variable type *suggests* that it's giving us some guarantee that it'll keep the thing alive, but it doesn't actually give us any extra guarantees over what we'd get from a raw pointer. If we happened to clear the array, for example, then touching |row| would be unsafe from that point on, regardless of its type.)

@@ +1143,5 @@
>  int32_t
>  nsTreeContentView::EnsureSubtree(int32_t aIndex)
>  {
> +  
> +  Row* row = mRows[aIndex].get();

Drop the first blank-line-with-whitespace here. (introduced in this patch)

@@ +1156,4 @@
>    int32_t index = 0;
>    Serialize(child, aIndex, &index, rows);
> +  // We can't use InsertElementsAt with an array argument since the
> +  // destination can't steal ownership from its const source argument.

While you're modifying this comment, please also clarify it a bit, to highlight *what we're doing* instead of what we're *not* doing.

E.g.:
  // Insert |rows| into |mRows| at position |aIndex|, by first creating empty
  // UniquePtr entries and then Move'ing |rows|'s entries into them. (Note
  // that we can't simply use InsertElementsAt with an array argument, since
  // [etc]

@@ +1238,5 @@
>    // We can't use InsertElementsAt since the destination can't steal
>    // ownership from its const source argument.
>    for (nsTArray<Row>::index_type i = 0; i < rows.Length(); i++) {
> +    UniquePtr<Row>* newRow = mRows.InsertElementAt(aParentIndex + aIndex + i + 1);
> +    *newRow = Move(rows[i]);

It'd be nice to make this into a safer one-liner while you're here. Can't we just do:
  mRows.InsertElementAt(aParentIndex + aIndex + i + 1,
                        Move(rows[i]);
?

That compiles for me locally, at least.
Attachment #8728572 - Flags: review?(dholbert) → review+

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/203f3bb029cf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.