1332105 - Convert SelectionDetails to use UniquePtr

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, SelectionDetails seems to be a linked list of raw pointers, each allocated with "new", which are cleaned up via manual "delete" calls.

Seems like a good opportunity for the RAII UniquePtr object, for safer ownership & leakproofing guarantees.

I'm noticing code like this, in particular:
=========
bool
nsTextFrame::PaintTextWithSelection([...SNIP...])
{
  NS_ASSERTION(GetContent()->IsSelectionDescendant(), "wrong paint path");

  SelectionDetails* details = GetSelectionDetails();
[...SNIP...]
  if (!PaintTextWithSelectionColors(aParams, details, &allRawSelectionTypes,
                                    aClipEdges)) {
    DestroySelectionDetails(details);
    return false;
  }
  [...SNIP...]
  for (size_t i = kSelectionTypeCount - 1; i >= 1; --i) {
    SelectionType selectionType = ToSelectionType(1 << (i - 1));
    if (selectionType & allRawSelectionTypes) {
      [...SNIP...]
    }
  }

  DestroySelectionDetails(details);
  return true;
}
=========
https://dxr.mozilla.org/mozilla-central/rev/80eac484366ad881c6a10bf81e8d9b8f7a676c75/layout/generic/nsTextFrame.cpp#6512

This looks like 'details' really wants to be managed in an RAII way.

Comment 1

[Daniel Holbert [:dholbert]]

(Note: in the code snippet above, GetSelectionDetails() seems to boil down to a call to "new SelectionDetails", plus some wrapping to manually construct a linked list of such objects. And DestroySelectionDetails just walks the chain and calls 'delete' on every entry.

So that "new" would end up being replaced with MakeUnique under the hood, and the DestroySelectionDetails call would become unnecessary because UniquePtr is RAII.)

Comment 2

[Daniel Holbert [:dholbert]]

TYLin, I notice you CC'd yourself -- if you're interested in taking this, feel free! :)

To anyone working on this: warning, I haven't traced through the entire SelectionDetails lifecycle, so it's possible they're shared or persisted in some way that makes this conversion impossible to do fully.  But at first glance, these objects seem uniquely-owned (and temporary) to me, so I think it's doable.

Basically:
 - All SelectionDetails* local-variables should be converted to UniquePtr<SelectionDetails>
 - The SelectionDetails struct's own member-variable (for the next link in the chain) should be converted to UniuePtr<SelectionDetails> as well.
 - All "new SelectionDetails" invocations should change to MakeUnique.
 - Function return-values / args / outparams should be converted to some flavor of UniquePtr<>, according to the recommendations in the documentation within UniquePtr.h.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Daniel, I'm sold :)

Let's see how try feels about my patch before requesting the review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e27dcf9c1af16274be4f013dcd4bf5d9124b3db

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1332105 Part 1 - Convert SelectionDetails and related functions to use UniquePtr.

Review commit: https://reviewboard.mozilla.org/r/106032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106032/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8828743 [details]
Bug 1332105 Part 1 - Convert SelectionDetails and related functions to use UniquePtr.

https://reviewboard.mozilla.org/r/106032/#review107114

Thanks for taking this on! This patch looks pretty close -- just a few requests:

::: layout/generic/nsSelection.cpp:4598
(Diff revision 1)
>  NS_IMETHODIMP
>  Selection::LookUpSelection(nsIContent* aContent, int32_t aContentOffset,
>                             int32_t aContentLength,
> -                           SelectionDetails** aReturnDetails,
> +                           UniquePtr<SelectionDetails>* aReturnDetails,
>                             SelectionType aSelectionType,
>                             bool aSlowCheck)
>  {

I'm a bit uneasy about this UniquePtr<>* in/outparam.  Happily, I think we can refactor this function a little (in a separate patch) to improve things!

This function returns nsresult, BUT DXR says this method only has one caller:
https://dxr.mozilla.org/mozilla-central/rev/aa3e49299a3aa5cb0db570532e3df9e75d30c2d1/layout/generic/nsSelection.cpp#1898
...and that caller ignores the nsresult return value anyway! :D  So we don't need the nsresult return type, in practice.  (Also, it's virtual from NS_IMETHOD/NS_IMETHODIMP, but I don't think it's overridden in practice, so we can devirtualize it as well.)

So: could you try splitting this in/outparam into...
  (a) a UniquePtr<> arg (no asterisk) (for the old head of the list)
  (b) a UniquePtr<> return value (for the new head of the list)

This should happen in a "part 2" patch, so that this first patch can remain a rewrite-in-place without much refactoring.

::: layout/generic/nsSelection.cpp:4664
(Diff revision 1)
> -    SelectionDetails* details = new SelectionDetails;
> +    auto details = MakeUnique<SelectionDetails>();
>  
> -    details->mNext = *aReturnDetails;
> +    details->mNext = Move(*aReturnDetails);
>      details->mStart = start;
>      details->mEnd = end;
>      details->mSelectionType = aSelectionType;
>      RangeData *rd = FindRangeData(range);
>      if (rd) {
>        details->mTextRangeStyle = rd->mTextRangeStyle;
>      }
> -    *aReturnDetails = details;
> +    *aReturnDetails = Move(details);

So, as part of the "part 2" refactor that I suggest above, I think this function's body would now become something like:

  // (near the beginning of the function body):
  UniquePtr<SelectionDetails> detailsHead = Move(aDetails);
  ...
  // (pre-existing for-loop):
  for (uint32_t i = 0; i < overlappingRanges.Length(); i++) {
    ...
    auto newHead = MakeUnique<SelectionDetails>();
    newHead->mNext = Move(detailsHead);
    ...
    detailsHead = Move(newHead);
  }
  return detailsHead;

::: layout/generic/nsTextFrame.cpp:6509
(Diff revision 1)
>    if (!details) {
>      return false;
>    }
>  
>    RawSelectionType allRawSelectionTypes;
> -  if (!PaintTextWithSelectionColors(aParams, details, &allRawSelectionTypes,
> +  if (!PaintTextWithSelectionColors(aParams, details.get(), &allRawSelectionTypes,

As part of this main patch, please update this PaintTextWithSelectionColors() function to take a
  |const UniquePtr<SelectionDetails>&|
...instead of a raw SelectionDetails pointer. (so that you don't need the get() call here.)

Any "get()" raw pointer usage is a potential footgun, for a UniquePtr-managed resource.  (This PaintText function could inadvertantly hold onto the raw pointer, or delete it, for example.  Or store it in its own local UniquePtr, which would then accidentally cause a double-free.  Judicious UniquePtr<> usage should prevent all of these issues by enforcing a single owner.)

::: layout/generic/nsTextFrame.cpp:6524
(Diff revision 1)
>      SelectionType selectionType = ToSelectionType(1 << (i - 1));
>      if (selectionType & allRawSelectionTypes) {
>        // There is some selection of this selectionType. Try to paint its
>        // decorations (there might not be any for this type but that's OK,
>        // PaintTextSelectionDecorations will exit early).
> -      PaintTextSelectionDecorations(aParams, details, selectionType);
> +      PaintTextSelectionDecorations(aParams, details.get(), selectionType);

Same here -- please fix this function to take a const UniquePtr&, so that you don't need the get() call here.

::: layout/generic/nsTextFrame.cpp:6608
(Diff revision 1)
> -  SelectionDetails* sdptr = details;
> +  SelectionDetails* sdptr = details.get();
>    SelectionType selectionType = SelectionType::eNone;
>    while (sdptr) {

Could you convert this to a "for" loop, like you've done for the other similar loops?  (That makes it slightly easier to reason about the scope of this |sdptr| raw-pointer, which makes me a little more comfortable about having it be a raw pointer.)

::: layout/generic/nsTextFrame.cpp:7215
(Diff revision 1)
> -  SelectionDetails* details = GetSelectionDetails();
> +  UniquePtr<SelectionDetails> details = GetSelectionDetails();
>    bool found = false;
>  
>    // where are the selection points "really"
> -  SelectionDetails *sdptr = details;
> +  SelectionDetails *sdptr = details.get();
>    while (sdptr) {

Same here -- this seems like it's asking to be converted to a for loop.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8828743 [details]
Bug 1332105 Part 1 - Convert SelectionDetails and related functions to use UniquePtr.

https://reviewboard.mozilla.org/r/106032/#review107114

> I'm a bit uneasy about this UniquePtr<>* in/outparam.  Happily, I think we can refactor this function a little (in a separate patch) to improve things!
> 
> This function returns nsresult, BUT DXR says this method only has one caller:
> https://dxr.mozilla.org/mozilla-central/rev/aa3e49299a3aa5cb0db570532e3df9e75d30c2d1/layout/generic/nsSelection.cpp#1898
> ...and that caller ignores the nsresult return value anyway! :D  So we don't need the nsresult return type, in practice.  (Also, it's virtual from NS_IMETHOD/NS_IMETHODIMP, but I don't think it's overridden in practice, so we can devirtualize it as well.)
> 
> So: could you try splitting this in/outparam into...
>   (a) a UniquePtr<> arg (no asterisk) (for the old head of the list)
>   (b) a UniquePtr<> return value (for the new head of the list)
> 
> This should happen in a "part 2" patch, so that this first patch can remain a rewrite-in-place without much refactoring.

Totally agree! Before reading your comment, I'm thinking about writing a part 2 which roughly does what you have suggested.

> As part of this main patch, please update this PaintTextWithSelectionColors() function to take a
>   |const UniquePtr<SelectionDetails>&|
> ...instead of a raw SelectionDetails pointer. (so that you don't need the get() call here.)
> 
> Any "get()" raw pointer usage is a potential footgun, for a UniquePtr-managed resource.  (This PaintText function could inadvertantly hold onto the raw pointer, or delete it, for example.  Or store it in its own local UniquePtr, which would then accidentally cause a double-free.  Judicious UniquePtr<> usage should prevent all of these issues by enforcing a single owner.)

Agree. Passing `const UniquePtr<SelectionDetails>&` rather than `SelectionDetalis*` to a function is better.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1332105 Part 2 - Make Selection::LookUpSelection() return UniquePtr.

Selection::LookUpSelection()'s only caller,
nsFrameSelection::LookUpSelection(), doesn't check the return value. Let's
make it return UniquePtr directly like nsFrameSelection::LookUpSelection().

Rename aReturnDetails to aDetailsHead so that it's role is clearer.
aDetailsHead is of type UniquePtr<SelectionDetails> instead of
UniquePtr<SelectionDetails>&& because the caller always wants to transfer
the ownership of SelectionDetails into the function.

Review commit: https://reviewboard.mozilla.org/r/106416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106416/

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8828743 [details]
Bug 1332105 Part 1 - Convert SelectionDetails and related functions to use UniquePtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106032/diff/1-2/

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8829316 [details]
Bug 1332105 Part 2 - Make Selection::LookUpSelection() return UniquePtr.

https://reviewboard.mozilla.org/r/106416/#review107614

r=me on part 2, with one minor nit on the extended commit message:

> Rename aReturnDetails to aDetailsHead so that it's role is clearer.

Please drop the apostrophe here -- should be "its".
( https://en.oxforddictionaries.com/usage/its-or-it-s )

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8828743 [details]
Bug 1332105 Part 1 - Convert SelectionDetails and related functions to use UniquePtr.

https://reviewboard.mozilla.org/r/106032/#review107618

r=me with some explanatory comments added in each of the PaintText*** functions, as requested below:

::: layout/generic/nsTextFrame.cpp:6253
(Diff revisions 1 - 2)
>    AutoTArray<SelectionDetails*,BIG_TEXT_NODE_SIZE> prevailingSelectionsBuffer;
>    SelectionDetails** prevailingSelections =

This "prevailingSelections" buffer *looks* quite problematic, because it's storing extra raw-pointers to uniquely-owned SelectionDetails instances.  (These get stored via "prevailingSelections[i] = sdptr;" assignments in the loop below.)

Fortunately, after further investigation, it seems this should be OK, because
 (a) this AutoTArray only lives for the duration of this function.
 (b) the things it points to are owned by a UniquePtr chain that the caller controls.
...which means the SelectionDetails objects are guaranteed to outlive these raw pointers.

So: I think this is all ok, BUT please add a comment here, explaining that this is storing raw pointers to UniquePtr-managed objects, but that it's safe, because [...].

::: layout/generic/nsTextFrame.cpp:6418
(Diff revisions 1 - 2)
>    AutoTArray<SelectionDetails*, BIG_TEXT_NODE_SIZE> selectedCharsBuffer;
>    SelectionDetails** selectedChars =
>      selectedCharsBuffer.AppendElements(contentRange.Length(), fallible);

Here as well, please add a comment to say that this object is keeping extra raw pointers to uniquely-owned resources, but that's OK because it's temporary and the resources are owned by the caller so they'll outlive this object.

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8828743 [details]
Bug 1332105 Part 1 - Convert SelectionDetails and related functions to use UniquePtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106032/diff/2-3/

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8829316 [details]
Bug 1332105 Part 2 - Make Selection::LookUpSelection() return UniquePtr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106416/diff/1-2/

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Uploaded a new patchset to address comment 9 and comment 10. Thank you for the review.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c66406fe3bdc
Part 1 - Convert SelectionDetails and related functions to use UniquePtr. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2b6b52a85c65
Part 2 - Make Selection::LookUpSelection() return UniquePtr. r=dholbert

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c66406fe3bdc
https://hg.mozilla.org/mozilla-central/rev/2b6b52a85c65