Switch dynamic CounterStyle objects to use arena allocation

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Per bug 1075336 comment 20, it is safer to allocate dynamic CounterStyle objects from pres shell arean instead of general heap.
(Assignee)

Comment 1

5 years ago
It seems that CustomCounterStyle objects could slightly outlive its PresShell:

In nsPresContext::SetShell, if the mShell is set to nullptr, CounterStyleManager->Disconnect() will be called, which releases all allocated CounterStyle objects. But at this point, no nsPresShell could be find via nsPresContext::PresShell(). Do you have any suggestion about this situation?
Flags: needinfo?(mats)
It seems that the CounterStyleManager keeps all its current data when
aShell != null in SetShell.  Doesn't that mean CounterStyleManager::mCacheTable
will still have some data associated with old shell?  This seems wrong
to me since some CounterStyle objects have Rule references (mRule).
(I admittedly don't understand how this stuff is supposed to work though)
http://hg.mozilla.org/mozilla-central/annotate/25a98bac9264/layout/base/nsPresContext.cpp#l1094


Why does the CounterStyleManager object live on the PresContext?
CounterStyleManager::mPresContext is mostly used only for
"mPresContext->StyleSet()" which just forwards to the shell where the
StyleSet lives.  It seems a bit fragile in case the shell changed.
It looks to me, when I read the CounterStyleManager code, that it's
more closely related to the StyleSet and its rule tree.  Perhaps we
could make it a member on the StyleSet instead? (and swap mPresContext
for a mStyleContext instead)  It seems simpler to create it in
nsStyleSet::Init and destroy it in Shutdown than to make the
PresContext manage it.
Flags: needinfo?(mats)
(Assignee)

Comment 3

5 years ago
(In reply to Mats Palmgren (:mats) from comment #2)
> It seems that the CounterStyleManager keeps all its current data when
> aShell != null in SetShell.  Doesn't that mean
> CounterStyleManager::mCacheTable
> will still have some data associated with old shell?  This seems wrong
> to me since some CounterStyle objects have Rule references (mRule).
> (I admittedly don't understand how this stuff is supposed to work though)
> http://hg.mozilla.org/mozilla-central/annotate/25a98bac9264/layout/base/nsPresContext.cpp#l1094

PresShell in fact owns nsPresContext, and a nsPresContext could be bound to at most one PresShell in its lifetime. Hence, "have some data associated with old shell" is never true. The only problem is that, when PresShell or nsPresContext is destroying, mPresShell will be set to nullptr before destroying the CounterStyleManager. I think the possible way is to put the destroying before setting mPresShell like mFontFaceSet.
(Assignee)

Comment 4

5 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → quanxunzhen
Attachment #8501365 - Flags: review?(mats)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #3)
> PresShell in fact owns nsPresContext,

No, nsDocumentViewer owns it.

> and a nsPresContext could be bound to
> at most one PresShell in its lifetime.

True, AFAICT. (although I don't think this was always the case
historically)

> Hence, "have some data associated with old shell" is never true.

Fair, but my point is that CounterStyleManager references style rules
directly, so even if there's only one shell, those pointers are
dangling after nsStyleSet::Shutdown is called.

> The only problem is that, when PresShell or
> nsPresContext is destroying, mPresShell will be set to nullptr before
> destroying the CounterStyleManager. I think the possible way is to put the
> destroying before setting mPresShell like mFontFaceSet.

Yes, that avoids the problem, but it's fragile, because if someone
changes this code, or adds a Shutdown() call at another place, there's
a risk we have an exploitable bug.  Also, nsPresContext shouldn't need
to know about these external dependences.

I think it would be better if nsStyleSet owns the CounterStyleManager
and manages it internally, making it *impossible* that an error can
occur because Shutdown() itself will disconnect it.  It isolates
these implementation details to the style system where it belongs.
I think we can do this refactoring in a follow-up bug though.
Comment on attachment 8501365 [details] [diff] [review]
patch

r=mats with a few minor nits:

>layout/style/CounterStyleManager.cpp
>+private:
>+  nsAutoRefCnt mRefCnt;
>+  NS_DECL_OWNINGTHREAD
>+  void Destroy()
>+  {
>+    this->~DependentBuiltinCounterStyle();
>+    mManager->PresContext()->PresShell()->FreeByObjectID(
>+        nsPresArena::DependentBuiltinCounterStyle_id, this);
>+  }
> 
> private:
>   CounterStyleManager* mManager;

In general, it seems cleaner to not use mManager after running the
destructor (but I guess we do that in other places too), like so:
nsIPresShell* shell = mManager->PresContext()->PresShell();
this->~DependentBuiltinCounterStyle();
shell->FreeByObjectID(...);

The second "private:" seems redundant. Also, could you move the
mRefCnt and NS_DECL_OWNINGTHREAD lines down next to mManager?
And ~DependentBuiltinCounterStyle() from the top of the class
next to Destroy(), so that private methods are grouped followed
by private fields.

Same comments for CustomCounterStyle.

This looks good otherwise, thanks for fixing this.

BTW, I see no reaon why AnonymousCounterStyle why can't also be
allocated from the pres shell arena, except that it doesn't have
a mManager member so can't get to the shell to deallocate.
Perhaps that's expensive to add though?
Attachment #8501365 - Flags: review?(mats) → review+
(Assignee)

Comment 7

5 years ago
(In reply to Mats Palmgren (:mats) from comment #6)
> Comment on attachment 8501365 [details] [diff] [review]
> patch
> 
> BTW, I see no reaon why AnonymousCounterStyle why can't also be
> allocated from the pres shell arena, except that it doesn't have
> a mManager member so can't get to the shell to deallocate.
> Perhaps that's expensive to add though?

The reason I don't make AnonymousCounterStyle allocated from the arena is the fact that there never exist raw pointers refer to it - all of them are RefPtr, since it doesn't have the cyclic reference problem.
I see, let's not bother with those then.
(Assignee)

Comment 9

5 years ago
Posted patch patch, r=matsSplinter Review
Attachment #8501365 - Attachment is obsolete: true
Attachment #8506490 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Hi Xidon, can you provide a try link for this changes ? thanks!
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d986a5a0ebed
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8506490 [details] [diff] [review]
patch, r=mats

Approval Request Comment
[Feature/regressing bug #]: bug 966166
[User impact if declined]: it's low risk and adds extra crash safety
[Describe test coverage new/current, TBPL]: on m-c since 2014-10-20
[Risks and why]: low risk
[String/UUID change made/needed]: none
Attachment #8506490 - Flags: approval-mozilla-beta?
Attachment #8506490 - Flags: approval-mozilla-aurora?
Comment on attachment 8506490 [details] [diff] [review]
patch, r=mats

Beta+
Aurora+
Attachment #8506490 - Flags: approval-mozilla-beta?
Attachment #8506490 - Flags: approval-mozilla-beta+
Attachment #8506490 - Flags: approval-mozilla-aurora?
Attachment #8506490 - Flags: approval-mozilla-aurora+

Comment 18

3 years ago
[Tracking Requested - why for this release]:
blocking-b2g: --- → 2.6?
Flags: in-testsuite+
Flags: in-qa-testsuite+
Flags: in-moztrap+
Flags: a11y-review+
(Assignee)

Updated

3 years ago
blocking-b2g: 2.6? → ---
Flags: in-testsuite+
Flags: in-qa-testsuite+
Flags: in-moztrap+
Flags: a11y-review+
You need to log in before you can comment on or make changes to this bug.