Closed Bug 1280887 Opened 3 years ago Closed 3 years ago

Consider making gfxTextRun a refcounted class

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(5 files)

Currently, ownership and lifetime management of gfxTextRun is a bit hairy, because gfxTextRun is not refcounted, but relies on being explicitly deleted when no longer required.[1]

Yet we often end up with multiple references to a given textrun (where lots of frames in a flow are all backed by a common textrun), so that knowing when a textrun should be deleted is non-trivial. And use-after-delete bugs involving textruns tend to be bad (potentially exploitable).

So I'm wondering whether it would be beneficial to make gfxTextRun a refcounted class, and hold strong references from all the textframes (or other code) using a given run. The main concern, I think, would be any potential perf impact, but I'm guessing this may be negligible.... talos should be able to confirm/deny that.

[1] https://dxr.mozilla.org/mozilla-central/rev/5f95858f8ddf21ea2271a12810332efd09eff138/gfx/thebes/gfxTextRun.h#71
Keywords: feature
Whiteboard: [gfx-noted]
Is it possible for us to be more disciplined with the ownership using UniquePtr? Given the existing code seems to mostly do this correctly I wonder if we can't just codify it's ownership better without having to go all the way to reference counting.
The current code uses UniquePtr while initially creating and passing around a gfxTextRun, and for transient runs used for some purposes; but that doesn't extend to the (primary) use of textruns by nsTextFrame instances, because a single textrun may be referenced by some arbitrary number of frames. At that point, we "forget" the UniquePtr ownership and just have a bunch of raw pointers in the various textframes (and/or in frame properties associated with them) all pointing to a single textrun. So in that area, I wonder if refcounting might provide us with simpler and more robust lifetime management than we currently have.
This is just a bit of preparatory cleanup that doesn't seem worth its own bug: there are a bunch of cases where we can declare gfxTextRun* variables as const, so we should do so as an aid to the reader (and maybe the compiler/optimizer). I expect there are probably some more yet, but this is a start, anyhow.
Attachment #8768889 - Flags: review?(mats)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Here's the basic change, adding refcounting to textruns. For the gfx code that creates them, this means replacing UniquePtr with RefPtr, but then once we hand them off to textframes, we were just using raw pointers and manually tracking when to delete them; giving textframes strong RefPtr references to their textruns instead should make things simpler/more robust.
Attachment #8768890 - Flags: review?(mats)
Once we're using RefPtrs for textruns, it looks like we don't need the whole mTextRunsToDelete thing any more, so let's kill it.
Attachment #8768891 - Flags: review?(mats)
I pushed a debug try run with the patches here to check that they don't end up introducing either leaks or crashiness, at least as far as our tests can tell: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf77185374e.
Comment on attachment 8768889 [details] [diff] [review]
patch 1 - Declare a bunch of gfxTextRun* variables as const pointers, for clarity

>gfx/thebes/gfxTextRun.h
>-    const GlyphRun *GetGlyphRuns(uint32_t *aNumGlyphRuns) {
>+    const GlyphRun *GetGlyphRuns(uint32_t *aNumGlyphRuns) const {

While we're here, please move the * next to the type name.
Attachment #8768889 - Flags: review?(mats) → review+
Comment on attachment 8768890 [details] [diff] [review]
patch 2 - Make gfxTextRun refcounted, replace usage of UniquePtr<> with RefPtr<> for textruns, and make nsTextFrame hold a strong reference to its run(s)

>gfx/thebes/gfxTextRun.cpp
>@@ -2095,56 +2096,56 @@ gfxFontGroup::MakeTextRun(const uint8_t 
>-    UniquePtr<gfxTextRun> textRun = gfxTextRun::Create(aParams, aLength,
>+    RefPtr<gfxTextRun> textRun = gfxTextRun::Create(aParams, aLength,
>                                                        this, aFlags);

Re-indent the last line.

>-    UniquePtr<gfxTextRun> textRun = gfxTextRun::Create(aParams, aLength,
>+    RefPtr<gfxTextRun> textRun = gfxTextRun::Create(aParams, aLength,
>                                                        this, aFlags);

Same here.

layout/generic/nsTextFrame.cpp
>@@ -2152,17 +2152,17 @@ BuildTextRunsScanner::BuildTextRunForFra

We should probably make BuildTextRunForFrames return 
already_AddRefed<gfxTextRun> too, and mCurrentFramesAllSameTextRun
a RefPtr.
Attachment #8768890 - Flags: review?(mats) → review+
Attachment #8768891 - Flags: review?(mats) → review+
It looks like gfxTextRun::ClusterIterator is unused and can be removed.
Here's a patch to remove it.
Feel free to include it here or file a follow-up bug.
(In reply to Mats Palmgren (:mats) from comment #9)
> Created attachment 8782699 [details] [diff] [review]
> remove-gfxTextRun-ClusterIterator
> 
> It looks like gfxTextRun::ClusterIterator is unused and can be removed.
> Here's a patch to remove it.
> Feel free to include it here or file a follow-up bug.

Huh, so it is. Great, I'll include this when I push these patches - thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/16ee64cae101f98f9a39787d27a7a61e7e77d350
Bug 1280887 - patch 1 - Declare a bunch of gfxTextRun* variables as const pointers, for clarity. r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0b12e4982eedfd9b336e8a3b196876af8b775b
Bug 1280887 - patch 2 - Make gfxTextRun refcounted, replace usage of UniquePtr<> with RefPtr<> for textruns, and make nsTextFrame hold a strong reference to its run(s). r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/7abbe47117a643f921319829da0f112718e81746
Bug 1280887 - patch 3 - Get rid of the mTextRunsToDelete array now that the frame's textruns are refcounted and will know when to die by themselves. r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2a5d5e9b5300367c596555ffa7ae78e682176b
Bug 1280887 - patch 4 - Remove (unused) class gfxTextRun::ClusterIterator. r=jfkthame
Hey Jonathan, is this something manual QA could help test?
Flags: qe-verify?
Flags: needinfo?(jfkthame)
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #13)
> Hey Jonathan, is this something manual QA could help test?

I don't think so. This is a purely internal change, intended to make the code more robust and maintainable, but there will (should!) be no change in behavior, so there's nothing specific for QA to verify.
Flags: needinfo?(jfkthame)
I just noticed we have a couple of places in gtest code where we use UniquePtr<gfxTextRun>, so those should be updated too -- although both these files are actually commented-out in the moz.build file, so they're currently dead code.
Attachment #8783423 - Flags: review?(mats)
Attachment #8783423 - Flags: review?(mats) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2d3c9808031734f59937dcc9a122035e8673f75
Bug 1280887 followup, replace UniquePtr<gfxTextRun> with RefPtr in test code (NPOTB). r=mats
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #13)
> > Hey Jonathan, is this something manual QA could help test?
> 
> I don't think so. This is a purely internal change, intended to make the
> code more robust and maintainable, but there will (should!) be no change in
> behavior, so there's nothing specific for QA to verify.

Thank you, Jonathan. Setting flags accordingly.
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.