Closed Bug 1162855 Opened 6 years ago Closed 6 years ago

FontFaceSet should traverse its mUserFontSet's mFontFaceSet back pointer

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file)

We currently don't traverse a FontFaceSet::mUserFontSet->mFontFaceSet.  It doesn't matter now, since in nsPresContext::Destroy we will call DestroyUserFontSet() on the FontFaceSet, which will clear both FontFaceSet::mUserFontSet->mFontFaceSet and FontFaceSet::mUserFontSet.  But in 1161413 I plan to allow FontFaceSets to remain usable even after (or without) a pres context goes away.
Attached patch patchSplinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8603186 - Flags: review?(bugs)
Comment on attachment 8603186 [details] [diff] [review]
patch

Hmm, I need to now go through the ownership model here.
Why are we explicitly traversing/unlinking a member variable of mUserFontSet.
Why can't we just traverse/unlink mUserFontSet, and let its implementation
to traverse/unlink mFontFaceSet?
FontFaceSet::UserfontSet inherits from gfxUserFontSet. And as I understand it no gfx classes are cycle collected and there is a desire to keep it that way.  I couldn't see a way to keep gfxUserFontSet being a plain recounted class but have its derived class be cycle collected.
*refcounted (sorry on phone)
But it is always FontFaceSet::mUserFontSet which owns one particular instance of FontFaceSet::UserfontSet? The instance isn't shared by multiple FontFaceSets?
(we must be sure to not traverse some field too many times, in this case mUserFontSet->mFontFaceSet)
Flags: needinfo?(cam)
Comment on attachment 8603186 [details] [diff] [review]
patch

Ok, looks like FontFaceSet::EnsureUserFontSet always creates a new UserFontSet per FontFaceSet object, so this should be fine.
Attachment #8603186 - Flags: review?(bugs) → review+
Thanks, yeah each FontFaceSet gets its own FontFaceSet::UserFontSet.
Flags: needinfo?(cam)
Patch doesn't have a commit message; guess I should check it in myself.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c2315e5bc62d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.