1283273 - Change nsAutoPtr to UniquePtr in classes within layout/generic

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Michael Li]

Attachment: Change nsAutoPtr to UniquePtr in generic layout classes

Comment 2

[Michael Li]

Attachment: Use MakeUnique function instead of reset in nsPluginFrame class

Comment 3

[Simon Montagu :smontagu]

Comment on attachment 8766516 [details] [diff] [review]
Change nsAutoPtr to UniquePtr in generic layout classes

Sorry, I can't do Mozilla code review right now

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8766516 [details] [diff] [review]
Change nsAutoPtr to UniquePtr in generic layout classes

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

Thanks for doing this cleanup work!

Nits below. They fall into three categories:
 - If we bother to store something in a UniquePtr, we should try to have it stored in a UniquePtr as far backwards/forwards as possible/convenient, in order for UniquePtr's guarantees to actually be effective.
 - We should strongly prefer MakeUnique() over new().
 - There are a few places where you're deleting the resource when really we want to transfer ownership (either by Move()'ing it into another UniquePtr, or with .release() to produce a raw pointer and clear out the UniquePtr.)

::: layout/generic/nsBlockFrame.cpp
@@ +4758,5 @@
>  
>  bool
>  nsBlockFrame::DrainSelfOverflowList()
>  {
> +  UniquePtr<FrameLines> ourOverflowLines(RemoveOverflowLines());

If possible, you should make UniquePtr propagate a bit further here -- it should be RemoveOverflowLines' return type, and RemoveOverflowLines should also use it as its local variable.  (UniquePtr works best when a resource is consistently stored in UniquePtr -- that reduces the risk of a double-free, leak, etc.)

(I don't think nsAutoPtr worked well as a return type, which is why RemoveOverflowLines just returns a raw pointer right now. But UniquePtr explicitly *does* work nicely as a return type -- so that's a worthwhile thing to consider when migrating from nsAutoPtr to UniquePtr.)

@@ +6539,1 @@
>      TextOverflow::WillProcessLines(aBuilder, this));

Same here -- ideally TextOverflow::WillProcessLines should return type UnqiuePtr as well, so that we're consistently storing this resource in a UniquePtr.

::: layout/generic/nsFlexContainerFrame.cpp
@@ +3436,5 @@
>          aStruts[nextStrutIdx].mItemIdx == itemIdxInContainer) {
>  
>        // Use the simplified "strut" FlexItem constructor:
> +      item.reset(new FlexItem(childFrame, aStruts[nextStrutIdx].mStrutCrossSize,
> +                          aReflowState.GetWritingMode()));

Two things:

(1) Please make sure the second line here lines up with the arguments on the first line (as it does in the old version of this code).

(2) I think you should be using:
  item = MakeUnique(...);
...instead of:
  item.reset(new FlexItem(...));

("myUniquePtr.reset(new Foo())" is probably something of an anti-pattern.  MakeUnique is preferred -- it's similar, shorter, & strictly better in various ways that are described in UniquePtr.h.)

@@ +3441,4 @@
>        nextStrutIdx++;
>      } else {
> +      item.reset(GenerateFlexItemForChild(aPresContext, childFrame,
> +                                      aReflowState, aAxisTracker));

As I noted for nsBlockFrame, we should propagate UniquePtr usage "virally" here, so that this resource is UniquePtr-tracked further back in its lifetime.  So, GenerateFlexItemForChild should return a UniquePtr instead of a raw pointer, and should use a UniquePtr local vairable (and should call MakeUnique instead of "new" internally).

Then I think this can just go back to being:
  item = GenerateFlexItemForChild(...)
or maybe
  item = Move(GenerateFlexItemForChild(...));

(I forget if you need to use Move() in this case or not. Only add that if you need it.)

@@ +3459,5 @@
>      }
>  
>      // Add item to current flex line (and update the line's bookkeeping about
>      // how large its items collectively are).
> +    curLine->AddItem(item.get(), shouldInsertAtFront,

.This should change to ".release()".  The point here is to explicitly leak the resource that we're tracking here (and trust that curLine will manage it safely from this point forward).

@@ +3464,3 @@
>                       itemInnerHypotheticalMainSize,
>                       itemOuterHypotheticalMainSize);
> +    item = nullptr;

This added line can/should be removed.

I'm pretty sure this will actually cause crashes / use-after-free -- this will delete the pointer managed by |item|, after we've given a copy of that pointer to |curLine| (which is now managing/using it)!

::: layout/generic/nsPluginFrame.cpp
@@ +1536,5 @@
>          mInstanceOwner->UseAsyncRendering())
>      {
>        RefPtr<ClientLayerManager> lm = aBuilder->GetWidgetLayerManager()->AsClientLayerManager();
>        if (!mDidCompositeObserver || !mDidCompositeObserver->IsValid(lm)) {
> +        mDidCompositeObserver.reset(new PluginFrameDidCompositeObserver(mInstanceOwner, lm));

You should just go straight to using UniquePtr here (i.e. you should fold your "patch 2" changes directly into this file).

This reset(new ...) version isn't particularly useful as an intermediate state -- it's simple enough to just go straight to the preferred MakeUnique expression.

::: layout/generic/nsTextFrame.cpp
@@ +737,5 @@
>      // Maybe the textrun was created for uninflated text.
>      return;
>    }
>  
> +  nsTArray<UniquePtr<GlyphObserver> >* observers =

Nit: while you're touching this, you can collapse "> >" to just ">>".

(The space is there for historical reasons -- older compilers couldn't handle ">>" in this context, because it looked like a stream operation, but modern C++/compilers can disambiguate the ">>" correctly.  And we prefer the more compact form, I believe.)

@@ +738,5 @@
>      return;
>    }
>  
> +  nsTArray<UniquePtr<GlyphObserver> >* observers =
> +    new nsTArray<UniquePtr<GlyphObserver> >();

Same here - while you're modernizing this line, please collapse to ">>".

@@ +2118,3 @@
>    if (anyTextTransformStyle) {
> +    transformingFactory.reset(
> +      new nsCaseTransformTextRunFactory(transformingFactory.get()));

transformingFactory = MakeUnique(...), please.

@@ +2121,4 @@
>    }
>    if (anyMathMLStyling) {
> +    transformingFactory.reset(
> +      new MathMLTextRunFactory(transformingFactory.get(), mathFlags,

(1) As above, use MakeUnique instead of reset(new(...))

(2) I believe the MathMLTextRunFactory() constructor is intended to be **explicitly taking ownership** of its first argument. (The old code caled "forget()" on the first arg, which explicitly gave up ownership here -- your new code here has "get()", which does not give up ownership. So this is subtly changing behavior in a way that might cause crashes or leaks, I think!!)  One hacky way of making this legitimate would be to call "release()" instead of "get()" here -- but even better would be to make MathMLTextRunFactory just directly take a UniquePtr as its first argument.  Then, I think you'd adjust this callsite to pass in Move(transformingFactory) as the first argument.

@@ +2171,5 @@
>                                                   &params, fontGroup, textFlags,
>                                                   Move(styles), true);
>        if (textRun) {
>          // ownership of the factory has passed to the textrun
> +        transformingFactory = nullptr;

As above, the patch is subtly changing behavior here! The "forget()" call in the old version of this code would clear the pointer, but not delete anything. In contrast, this new code will clear the pointer **and delete the thing that it's pointing to**.

Please use ".release()" here instead of assigning to nullptr.   (That will be pretty much equivalent to what the old ".forget()" call was doing.)

(This is admittedly a bit hacky/sub-optimal -- the ideal way to clean this up would be to pass in Move(transformingFactory) at the point where ownership-transfer happens. Unfortunately, it looks like the ownership transfer happens inside of a method that's *called on the object whose ownership is being transferred (transformingFactory->MakeTextRun(...)). That's a bit bizarre. So, there's no easy way to fix this the right way without refactoring that method and the stack of functions that it calls -- so I wouldn't bother at this point.)

@@ +2186,5 @@
>                                                   &params, fontGroup, textFlags,
>                                                   Move(styles), true);
>        if (textRun) {
>          // ownership of the factory has passed to the textrun
> +        transformingFactory = nullptr;

As above, please use "transformingFactory.release()" here -- don't assign to nullptr (since that'll delete the value, which is now being managed by someone else).

@@ +2389,5 @@
>      iterNext.AdvanceOriginal(mappedFlow->GetContentEnd() -
>              mappedFlow->mStartFrame->GetContentOffset());
>  
> +    UniquePtr<BreakSink>* breakSink(
> +      mBreakSinks.AppendElement(new BreakSink(aTextRun, mDrawTarget, offset)));

Two nits:
(1) The outermost assignment here (breakSink) is initializing a raw pointer value, but the new "(...)" syntax that you're using makes it look like it's invoking a constructor or something. Please just stick with "=".

(2) As noted in other comments here, "MakeUnique" should be strongly preferred over "new", for initializing UniquePtrs. Unfortunately MakeUnique(...) on its own doesn't quite work here, since the compiler can't infer the type for some reason. So, you'll need to specify it explicitly -- this works for me:
    UniquePtr<BreakSink>* breakSink =
      mBreakSinks.AppendElement(MakeUnique<BreakSink>(aTextRun, mDrawTarget,
                                                      offset));


(3) Slight tangent -- the null-check that follows this code -- "if (!breakSink || !*breakSink)" -- is vestigial and unneeded these days (since "new/MakeUnique" and AutoTArray (the array here) are all infallible by default -- i.e. they'll simply crash safely if they fail.)  I'll file a followup on removing that null-check.  At that point, I think we'll be able to remove |breakSink| as well, since it's only used for that null-check.

::: layout/generic/nsTextRunTransformations.h
@@ +107,5 @@
>                                nsTArray<uint8_t>* aCanBreakBeforeArray = nullptr,
>                                nsTArray<RefPtr<nsTransformedCharStyle>>* aStyleArray = nullptr);
>  
>  protected:
> +  mozilla::UniquePtr<nsTransformingTextRunFactory> mInnerTransformingTextRunFactory;

The line after this (bool ... mAllUppercase;) gets left with an awkward amount of indentation -- it's indented to where this member-variable was previously indented to.

Please adjust it, so that it's either still-aligned, or so that it doesn't have any bonus indentation at all (just "bool mAllUppercase").  Either is fine.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8766899 [details] [diff] [review]
Use MakeUnique function instead of reset in nsPluginFrame class

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

r- on "part 2" because we should just do this up-front in part 1.

(i.e. we should go straight from "= new" in the old code to "= MakeUnique" in the new code. I don't see any benefit from doing this as a multi-step change with ".reset(new...)" as an intermediate state.)

Comment 6

[Daniel Holbert [:dholbert]]

One more nit, on the commit message in part 1:
> bug 1283273 - Change nsAutoPtr to UniquePtr in generic layout classes

"generic layout classes" is probably not the clearest term for these classes. That term makes me think of "Generic Classes" in the sense of STL containers & Java Generics, etc.  These classes do happen to *live* in a directory called "layout/generic", but that directory name really just means "grab-bag" ("generic" as opposed to "tables", "forms", etc).  The classes themselves aren't really "generic" in any meaningful sense.

So: I'd prefer that the commit message said "various layout classes", or "in classes within layout/generic"

Comment 7

[Daniel Holbert [:dholbert]]

(I filed bug 1283673 for the "slight tangent" in comment 4, BTW.)

Comment 8

[Michael Li]

Attachment: Change nsAutoPtr to UniquePtr in classes within layout/generic

Comment 9

[Michael Li]

Thanks for all your feedback! I've put all my changes into one patch.
I wasn't able to change the RemoveOverFlowLines() function and all the code in nsBlockFrame.cpp that referenced it to use UniquePtr; the code compiled but it didn't pass the tests in layout/generic/test/.
I also wasn't able to change TextOverflow::WillProcessLines() to use UniquePtr; something like
>return MakeUnique<TextOverflow>(aBuilder, aBlockFrame);
said that the TextOverflow constructor was protected so it couldn't be accessed from MakeUnique, and making a temporary UniquePtr of type TextOverflow like so:
>UniquePtr<TextOverflow> textOverflow(new TextOverflow(aBuilder, aBlockFrame));
>return textOverflow;
didn't work either.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8767757 [details] [diff] [review]
Change nsAutoPtr to UniquePtr in classes within layout/generic

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

Thanks for fixing this up -- this is getting closer!  Still a few things to fix, though:

::: layout/generic/nsBlockFrame.cpp
@@ +4758,5 @@
>  
>  bool
>  nsBlockFrame::DrainSelfOverflowList()
>  {
> +  UniquePtr<FrameLines> ourOverflowLines(RemoveOverflowLines());

Per my first two review notes in comment 4, we should change RemoveOverflowLines and WillProcessLines to return "UniquePtr" types as well... that doesn't seem to be addressed in this patch version.

However, on further thought, that's perhaps best done as a separate patch anyway.

SO: could you please add a "part 2" here that makes RemoveOverflowLines and WillProcessLines to return type |UniquePtr| (and use local UniquePtr variables as-needed), to push the UniquePtr-ness one level deeper here?

(As I noted in comment 4,  UniquePtr works best when a resource is *consistently* stored in UniquePtr, for as much of its life as possible.  It's awkward for a function to return a raw pointer which we then store in a UniquePtr. That's what we had to do with nsAutoPtr in old code, but UniquePtr gives us more power to enforce this via return types.)

::: layout/generic/nsFlexContainerFrame.cpp
@@ +1286,5 @@
>      }
>    }
>  
>    // Construct the flex item!
> +  UniquePtr<FlexItem> item = MakeUnique<FlexItem>(childRS,

Two things:
 (1) Please replace this line with:
  auto item = MakeUnique<FlexItem>(childRS,

(UniquePtr.h recommends using "auto" with MakeUnique where possible -- i.e. when it's being declared/assigned in a single line -- and we might as well do that here to avoid making this line extra-long and repeating ourselves w.r.t. Unique & FlexItem on the same line.)

 (2) Please reindent the args on subsequent lines here to keep them aligned with "childRS" here.

::: layout/generic/nsTextFrame.cpp
@@ +2118,3 @@
>    if (anyTextTransformStyle) {
>      transformingFactory =
> +      MakeUnique<nsCaseTransformTextRunFactory>(transformingFactory.get());

Please use Move(transformingFactory) here -- like you do for the MathML case a few lines further down -- and change the nsCaseTransformTextRunFactory constructor and member-variable to use UniquePtr as well.  (Right now it looks like nsCaseTransformTextRunFactory uses a raw-pointer argument, and a nsAutoPtr member-variable.)

That will make the ownership transfer more explicit, and it will also make it clearer what's happening to the old |transformingFactory| when we assign it out from under itself here. (which is a bit awkward)

@@ +2125,1 @@
>                                 sstyScriptLevel, fontInflation);

This line needs reindenting to stay aligned with the line before (which you're changing).

@@ +2171,5 @@
>                                                   &params, fontGroup, textFlags,
>                                                   Move(styles), true);
>        if (textRun) {
>          // ownership of the factory has passed to the textrun
> +        transformingFactory.release();

This doesn't quite compile for me locally. In particular, this release() call causes a build warning, which is treated as an error because I have ac_add_options --enable-warnings-as-errors in my mozconfig (like our TreeHerder machines do). You should consider adding that to your mozconfig as well.

We get a build warning here because UniquePtr::release() is declared as MOZ_MUST_USE -- it's declared that way because it would effectively be leaking a uniquely-owned object, if nobody else were already tracking that object [and generally, it's reasonable to expect that nobody else is tracking it, since it's supposed to be uniquely-owned].  BUT, in this case, we already know someone else has already taken ownership of the object (inside of MakeTextRun) -- so dropping the release() return value on the floor is OK.  So, we should *suppress* the build warning here, for now.

So, to address this -- please do the following 3 things:
 (1) Pass the release() return value to "Unused", which is our convention for suppressing this warning. i.e. replace the line with this:
   Unused << transformingFactory.release();

(You'll need to add #include "mozilla/unused.h" to provide this, too. Yes, the header is lowercase but the class is uppercase. Not sure why; historical reasons probably.)

 (2) File a followup bug on cleaning up the factory's ownership transfer here (which currently awkwardly happens inside of  the transformingFactory->MakeTextRun call. We should perhaps change MakeTextRun() and its helpers to take a UniquePtr<>& argument, so that it can *explicitly* steal ownership as-needed when it succeeds.)

 (3) Add an XXX or TODO code-comment just before this line, mentioning that we should clean up this ownership transfer & mention the bug number for the followup bug that you've filed.  (This "unused"/"release()" hackiness is horrible, but it's OK as a conversion from our existing similarly-hacky code.  But we should not leave this code in this state forever.)

@@ +2186,5 @@
>                                                   &params, fontGroup, textFlags,
>                                                   Move(styles), true);
>        if (textRun) {
>          // ownership of the factory has passed to the textrun
> +        transformingFactory.release();

Same here -- please add "Unused <<", and add the same code-comment that I suggested above (with a bug number).

Comment 11

[Daniel Holbert [:dholbert]]

Oops, sorry! I only just now saw comment 9. That explains why the OverflowLines functions weren't changed in your latest patch - sorry for missing that.

(In reply to Michael Li from comment #9)
> I wasn't able to change the RemoveOverFlowLines() function and all the code
> in nsBlockFrame.cpp that referenced it to use UniquePtr; the code compiled
> but it didn't pass the tests in layout/generic/test/.

Hmm, could you maybe spin off a followup bug for this, then? ("Make RemoveOverflowLines() return a UniquePtr") And mark it as depending on this bug.

I'll bet this should be doable -- there's probably a subtle typo in the changes you had to attempt this conversion, which we could figure out with some debugging but which doesn't need to block the rest of the changes here.

> I also wasn't able to change TextOverflow::WillProcessLines() to use
> UniquePtr; something like
> >return MakeUnique<TextOverflow>(aBuilder, aBlockFrame);
> said that the TextOverflow constructor was protected so it couldn't be
> accessed from MakeUnique

Darn! I expect we just need a special "friend" declaration, to give the correct templated MakeUnique function access to the private constructor.

Could you file a followup for this, too?  ("Make TextOverflow::WillProcessLines() return a UniquePtr")

Comment 12

[Daniel Holbert [:dholbert]]

(Just to be clear: don't feel pressured to assign any of the followup bugs to yourself -- I just want to be sure we've got them filed so we don't forget about these things, but I don't mean to imply that you have to fix those bugs as well.  But if you want to fix them, that's great too! :) )

Comment 13

[Michael Li]

Attachment: Change nsAutoPtr to UniquePtr in classes within layout/generic

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8768887 [details] [diff] [review]
Change nsAutoPtr to UniquePtr in classes within layout/generic

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

This looks great -- thanks!

r=me, with one nit fixed. (and assuming this passes a reftests + mochitests + crashtests TryServer run, with a debug build)

::: layout/generic/nsFlexContainerFrame.cpp
@@ +1288,5 @@
>  
>    // Construct the flex item!
> +  auto item = MakeUnique<FlexItem>(childRS,
> +                                  flexGrow, flexShrink, flexBaseSize,
> +                                  mainMinSize, mainMaxSize,

Nit: all the lines below MakeUnique here need 1 more space of indentation, so that the arguments line up (e.g. the first letter of "flexGrow" should line up directly underneath the first letter of "childRS")

Comment 15

[Michael Li]

I did a TryServer run with the last patch, which is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33eed766fd15

Comment 16

[Michael Li]

Attachment: Change nsAutoPtr to UniquePtr in classes within layout/generic

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8768966 [details] [diff] [review]
Change nsAutoPtr to UniquePtr in classes within layout/generic

Thanks! r=me

(Note: if you wanted, you could have set the r+ flag yourself here, and said "carrying forward r=dholbert", since you were just addressing the nit from my comment where I'd already granted r+.

Not a big deal, just letting you know for future reference -- when there's only one or two minimal changes, reviewers will often mark a patch as r+ with some nits fixed, and then you can feel free to just repost and not bother re-requesting review [unless you feel like there's something that needs another round of review])

Thanks again for doing this cleanup work - it's great to have this code modernized!

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8768887 [details] [diff] [review]
Change nsAutoPtr to UniquePtr in classes within layout/generic

(Also, generally you should mark old patch-versions as obsolete when you upload newer versions of that patch, to make it easier to see what patch is currently relevant for review/landing/etc. --> I'm doing that now, for most recent patch-version.)

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Michael Li from comment #15)
> I did a TryServer run with the last patch, which is here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=33eed766fd15

Great! The failures there all look intermittent/unrelated. (and it looks like the only changes between that & the latest patch here are in whitespace/comments, so no need to re-test)  You can go ahead and put "checkin-needed" in this bug's "keyword" field, and someone should come along and land this for you in the next day or two.

Comment 20

[Daniel Holbert [:dholbert]]

Actually, before adding checkin-needed -- could you please edit the commit message to include "r=dholbert" at the and, and repost the patch? That way, a sheriff can come along and land the patch with some other ones, without having to worry about hand-editing anything in your patch.

(Developers will often include "r?whoever" in the commit message up-front, so that you're indicating who the reviewer is without saying that review's been granted yet.  Sheriffs will generally catch that & swap out the "?" for an "=" as-appropriate when landing your patch. In this case you can just jump straight to "r=dholbert" since you're reposting after review's already been granted)

Comment 21

[Michael Li]

Attachment: Bug 1283273 - Change nsAutoPtr to UniquePtr in classes within layout/generic. r=dholbert

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8769209 [details] [diff] [review]
Bug 1283273 - Change nsAutoPtr to UniquePtr in classes within layout/generic. r=dholbert

Thanks! (I'll explicitly mark this final version r+, just to make sure the sheriff who lands this can easily see that it's been reviewed.)

Comment 23

[Michael Li]

Thanks for all your help Daniel!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/616e6e57a7df
Change nsAutoPtr to UniquePtr in classes within layout/generic. r=dholbert

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/616e6e57a7df