Closed Bug 1291483 Opened 8 years ago Closed 7 years ago

gfxFont should use MakeUnique instead of "new" when initializing UniquePtr values

Categories

(Core :: Graphics: Text, defect)

Product:
Core
Component:
Graphics: Text
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

Bug 1291483 part 1: Use 'MakeUnique' instead of 'new' to allocate gfxFont::mGlyphChangeObservers.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1291483 part 2: Use UniquePtr/MakeUnique more thoroughly in chain-of-custody for gfxFont::mVerticalMetrics.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
gfxFont.h and gfxFont.cpp have several UniquePtr variables that are currently initialized like so:

>    UniquePtr<gfxTextContextPaint> contextPaint;
[...]
>        contextPaint.reset(
>            new SimpleTextContextPaint(fillPattern, nullptr,
>                                       aRunParams.context->CurrentMatrix()));

That reset()/new should be replaced with something like:
>        contextPaint =
>          MakeUnique<SimpleTextContextPaint(fillPattern, nullptr,
>                                            aRunParams.context->CurrentMatrix()));

(MakeUnique is strictly better/safer than "new", for variables that are owned/managed via UniquePtr.)
The variables in question are the UniquePtr values declared in
 https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h
 https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp
...and reset() using "new" in gfxFont.cpp.

They should be replaced with MakeUnique expressions as shown in comment 0.

Whoever works on this bug should read through the UniquePtr documentation first, to familiarize themselves with how UniquePtr objects work:
https://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h
Hi Daniel, Based on your recommendation I would like to work on this bug :)
Flags: needinfo?(dholbert)
Thanks! I've assigned the bug to you.
Assignee: nobody → theeviltwin
Flags: needinfo?(dholbert)
Note that if we move ahead with bug 1280887, this will be obsoleted (or at least drastically changed), so it might be good to hold off for a bit, pending review of the patches there.
Ah, thank you. @theeviltwin, probably best not to bother with this bug at the moment then.  Sorry for sending you to a dead-end bug. :)

(Maybe there will still be some work to do here after bug 1280887 reworks this code, though...)
Assignee: theeviltwin → nobody
No problem, Daniel. I'll keep an eye on 1280887 and come back to this when that is concluded. Meanwhile, do tell me anytime if you feel there is one I can work on. Thanks :)
Whiteboard: [gfx-noted]
Pranaydeep, bug 1280887 is fixed now, and it looks like there is still some work to do on this bug. Are you still interested in working on it?
Flags: needinfo?(theeviltwin)
Pranaydeep hasn't been active since November, so I guess not.
Flags: needinfo?(theeviltwin)
Since the scope here has migrated a bit (and it's harder for a new contributor to figure out what needs doing than it perhaps was originally), I'll just take this.
Assignee: nobody → dholbert
After these patches, we still have one more UniquePtr initialization with reset(), for mShapedWord:
https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/gfx/thebes/gfxFont.cpp#2599-2601

I'm leaving that one be, because its allocation involves (fallible) malloc + placement "new".  It might be possible to convert that to use MakeUnique, but I'm not sure & it'd be a more delicate/involved operation.
Comment on attachment 8845600 [details]
Bug 1291483 part 1: Use 'MakeUnique' instead of 'new' to allocate gfxFont::mGlyphChangeObservers.

https://reviewboard.mozilla.org/r/118726/#review120702
Attachment #8845600 - Flags: review?(jfkthame) → review+
Comment on attachment 8845601 [details]
Bug 1291483 part 2: Use UniquePtr/MakeUnique more thoroughly in chain-of-custody for gfxFont::mVerticalMetrics.

https://reviewboard.mozilla.org/r/118728/#review120704
Attachment #8845601 - Flags: review?(jfkthame) → review+
Comment on attachment 8845601 [details]
Bug 1291483 part 2: Use UniquePtr/MakeUnique more thoroughly in chain-of-custody for gfxFont::mVerticalMetrics.

https://reviewboard.mozilla.org/r/118728/#review120754

::: gfx/thebes/gfxFont.cpp:3645
(Diff revision 1)
> -const gfxFont::Metrics*
> +UniquePtr<const gfxFont::Metrics>
>  gfxFont::CreateVerticalMetrics()
>  {
>      const uint32_t kHheaTableTag = TRUETYPE_TAG('h','h','e','a');
>      const uint32_t kVheaTableTag = TRUETYPE_TAG('v','h','e','a');
>      const uint32_t kPostTableTag = TRUETYPE_TAG('p','o','s','t');
>      const uint32_t kOS_2TableTag = TRUETYPE_TAG('O','S','/','2');
>      uint32_t len;
>  
> -    Metrics* metrics = new Metrics;
> +    UniquePtr<Metrics> metrics = MakeUnique<Metrics>();

Drat, Try complains about the fact that these two UniquePtr types don't quite agree. (The function's declared return-type has "const" on the value inside the angle-brackets, whereas the local variable does not.)

This Just Works on my machine with clang trunk, but it doesn't work on Try (with older clang on Mac and with gcc on Linux).

I'll sort this out before landing...
Discussed this with Waldo on IRC, and "return Move(metrics)" is the appropriate fix for the issue in comment 15.  It works as shown in this updated Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba09bc12867e56978cdb7b4de0b509f1131e6d6d

I'll go ahead and trigger autoland, now that that's been fixed.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a0c4791eef5
part 1: Use 'MakeUnique' instead of 'new' to allocate gfxFont::mGlyphChangeObservers. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/c8bfaf1927b6
part 2: Use UniquePtr/MakeUnique more thoroughly in chain-of-custody for gfxFont::mVerticalMetrics. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/5a0c4791eef5
https://hg.mozilla.org/mozilla-central/rev/c8bfaf1927b6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
reopen, this caused https://bugzilla.mozilla.org/show_bug.cgi?id=1346215 and so got backed out from central in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=528e9dbbb882db0b32792d44b5be9cc539afa1a8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(dholbert)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/fc58b761a05b
Backed out changeset c8bfaf1927b6
(In reply to Carsten Book [:Tomcat] from comment #21)
> reopen, this caused https://bugzilla.mozilla.org/show_bug.cgi?id=1346215

When aja was investigating this crash last night, he determined that crash was introduced before this landed. So I'm pretty sure this is good to re-land, but I'll wait a day or two until we've sorted out the startup crash.
No longer depends on: 1346215
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08753d5d8f1a
part 1: Use 'MakeUnique' instead of 'new' to allocate gfxFont::mGlyphChangeObservers. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/2f96904d7b3a
part 2: Use UniquePtr/MakeUnique more thoroughly in chain-of-custody for gfxFont::mVerticalMetrics. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/08753d5d8f1a
https://hg.mozilla.org/mozilla-central/rev/2f96904d7b3a
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Flags: needinfo?(dholbert) → in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: