Closed Bug 1265459 Opened 8 years ago Closed 8 years ago

Use UniquePtr instead of nsAutoPtr to hold onto gfxTextRuns

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(1 file, 2 obsolete files)

As a followup to bug 1265452, we have a bunch of places where we use nsAutoPtr<gfxTextRun>; let's update them to UniquePtr. And as part of that, change MakeTextRun and similar methods to directly return a UniquePtr instead of a raw pointer. One small step towards more explicit tracking of who owns what...
Let's see how far tryserver gets with it...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0c03cb12e5
Comment on attachment 8742424 [details] [diff] [review]
Replace uses of nsAutoPtr<gfxTextRun> with UniquePtr, and let MakeTextRun and similar methods return a UniquePtr

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

I much prefer the idiom:
if (!a)
  return nullptr;

to:
if (!a)
  return a;

::: gfx/tests/gtest/gfxFontSelectionTest.cpp
@@ +263,5 @@
>      uint32_t length;
>      if (test->stringType == S_ASCII) {
>          flags |= gfxTextRunFactory::TEXT_IS_ASCII | gfxTextRunFactory::TEXT_IS_8BIT;
>          length = strlen(test->string);
> +        textRun = Move(fontGroup->MakeTextRun(

MakeTextRun returns an r-value reference so Move is unneeded.

@@ +273,1 @@
>      }

Same here.

::: gfx/thebes/gfxTextRun.cpp
@@ +142,4 @@
>      }
>  
> +    result.reset(new (storage) gfxTextRun(aParams, aLength, aFontGroup, aFlags));
> +    return result;

I think this would be better as:
return UniquePtr<gfxTextRun>(new (storage) gfxTextRun(aParams, aLength, aFontGroup, aFlags));

and just continuing to return nullptr in the failure case.

@@ +2571,2 @@
>          MakeTextRun(ellipsis.get(), ellipsis.Length(), &params,
>                      aFlags | TEXT_IS_PERSISTENT, nullptr);

I'd prefer if this was kept more similar to the old code.

::: layout/mathml/nsMathMLChar.cpp
@@ +1283,5 @@
>                                 : bm.rightBearing - bm.leftBearing;
>      } else {
>        // Null glue indicates that a rule will be drawn, which can stretch to
>        // fill any space.
> +      textRun[i].reset(nullptr);

= nullptr should be njust fine.
Attachment #8742424 - Flags: review?(jmuizelaar) → review-
Attachment #8742424 - Attachment is obsolete: true
Comment on attachment 8742473 [details] [diff] [review]
Replace uses of nsAutoPtr<gfxTextRun> with UniquePtr, and let MakeTextRun and similar methods return a UniquePtr

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

::: gfx/tests/gtest/gfxTextRunPerfTest.cpp
@@ +84,5 @@
>          flags |= gfxTextRunFactory::TEXT_IS_ASCII |
>                   gfxTextRunFactory::TEXT_IS_8BIT;
>          length = strlen(test->mString);
> +        textRun = Move(fontGroup->MakeTextRun(
> +            reinterpret_cast<const uint8_t*>(test->mString), length, &params, flags));

Drop the Move.

@@ +89,4 @@
>      } else {
>          NS_ConvertUTF8toUTF16 str(nsDependentCString(test->mString));
>          length = str.Length();
> +        textRun = Move(fontGroup->MakeTextRun(str.get(), length, &params, flags));

Drop the Move.

::: gfx/tests/gtest/gfxWordCacheTest.cpp
@@ +70,2 @@
>    } else {
> +    textRun = Move(aFontGroup->MakeTextRun(aText, aLength, aParams, aFlags));

Drop all of the Move()s

::: gfx/thebes/gfxTextRun.cpp
@@ +2570,3 @@
>          MakeTextRun(ellipsis.get(), ellipsis.Length(), &params,
>                      aFlags | TEXT_IS_PERSISTENT, nullptr);
> +    if (!mCachedEllipsisTextRun) {

The previous code does not replace mCachedEllipsisTextRun if MakeTextRun returns nullptr. Is this change of behaviour intentional?

::: layout/generic/nsTextFrame.cpp
@@ +2274,5 @@
>      textRun->SetUserData(nullptr);
>      DestroyUserData(userDataToDestroy);
> +    // Arrange for this textrun to be deleted the next time the linebreaker
> +    // is flushed out
> +    mTextRunsToDelete.AppendElement(textRun.release());

Can you change mTextRunsToDelete into an AutoTArray<UniquePtr<gfxTextRun>,5> and then do a Move() here?

@@ +2281,5 @@
>  
>    // Actually wipe out the textruns associated with the mapped frames and associate
>    // those frames with this text run.
> +  AssignTextRun(textRun.get(), fontInflation);
> +  return textRun.release();

This is scary here. Where does the ownership of the textRun go?
Attachment #8742473 - Flags: review?(jmuizelaar) → review-
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> ::: gfx/thebes/gfxTextRun.cpp
> @@ +2570,3 @@
> >          MakeTextRun(ellipsis.get(), ellipsis.Length(), &params,
> >                      aFlags | TEXT_IS_PERSISTENT, nullptr);
> > +    if (!mCachedEllipsisTextRun) {
> 
> The previous code does not replace mCachedEllipsisTextRun if MakeTextRun
> returns nullptr. Is this change of behaviour intentional?

Yes, I noticed that, but it seemed more sensible to do this. The fontgroup only ever attempts to cache a "current" ellipsis textrun -- i.e. the ellipsis for the current device resolution & zoom level. AFAIK, the only reason MakeTextRun would fail is if we're desperately low on memory (and given how short the ellipsis text usually is, this is really far-fetched!), but if it _does_ ever fail, I don't see that keeping a no-longer-current ellipsis textrun hanging around is helping anybody.

(If this ever happens, the browser is most likely about to die with an OOM abort anyway, but that's secondary...)
Comment on attachment 8742473 [details] [diff] [review]
Replace uses of nsAutoPtr<gfxTextRun> with UniquePtr, and let MakeTextRun and similar methods return a UniquePtr

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

::: layout/generic/nsTextFrame.cpp
@@ +2274,5 @@
>      textRun->SetUserData(nullptr);
>      DestroyUserData(userDataToDestroy);
> +    // Arrange for this textrun to be deleted the next time the linebreaker
> +    // is flushed out
> +    mTextRunsToDelete.AppendElement(textRun.release());

That's probably possible, but I'd prefer not to do it here in this patch. gfxTextRun ownership in the nsTextFrame world gets hairy, what with the gTextRuns cache, timed expiration, and so on; I'd feel better leaving that for a separate bug.

(Actually, I have a suspicion the FrameTextRunCache is a vestige of an old caching scheme that we're no longer using, and could perhaps be killed off altogether. That would be cool! I'll look into it.)

@@ +2281,5 @@
>  
>    // Actually wipe out the textruns associated with the mapped frames and associate
>    // those frames with this text run.
> +  AssignTextRun(textRun.get(), fontInflation);
> +  return textRun.release();

Same place it used to go when we were using a raw gfxTextRun* here. We're just making the scariness more obvious now. :)

As I understand it, at this point the textrun "belongs" to the frame(s) for which we've been building it.... note the method name here, BuildTextRunForFrames (plural). gfxTextRun isn't reference counted -- presumably for perf reasons -- so lifetime management is indeed scary once we start sharing them like this... maybe something to reconsider, some day. (Yes, it's been a source of bugs.)
Ugh, sorry -- those "review" comments were meant as replies to the last two questions in comment 5.
Attachment #8742473 - Attachment is obsolete: true
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> ::: layout/generic/nsTextFrame.cpp
> @@ +2274,5 @@
> >      textRun->SetUserData(nullptr);
> >      DestroyUserData(userDataToDestroy);
> > +    // Arrange for this textrun to be deleted the next time the linebreaker
> > +    // is flushed out
> > +    mTextRunsToDelete.AppendElement(textRun.release());
> 
> Can you change mTextRunsToDelete into an AutoTArray<UniquePtr<gfxTextRun>,5>
> and then do a Move() here?

Just FTR, I've put this as a followup patch in bug 1265648.
Attachment #8742684 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e61ba81ac8c2cb687dc905298b4390086ce2f83
Bug 1265459 - Replace uses of nsAutoPtr<gfxTextRun> with UniquePtr, and let MakeTextRun and similar methods return a UniquePtr. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/3e61ba81ac8c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.