Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 10 years ago

Per bug 1075336 comment 20, it is safer to allocate dynamic CounterStyle objects from pres shell arean instead of general heap.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 1 • 10 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?

	Mats Palmgren (inactive)
	Comment 2 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 3 • 10 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.

	Mats Palmgren (inactive)
	Comment 5 • 10 years ago

(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.

	Mats Palmgren (inactive)
	Comment 6 • 10 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 7 • 10 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.

	Mats Palmgren (inactive)
	Comment 8 • 10 years ago

I see, let's not bother with those then.

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

Hi Xidon, can you provide a try link for this changes ? thanks!

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 11 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fbd0d54a3248

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d986a5a0ebed

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d986a5a0ebed

	Mats Palmgren (inactive)
	Comment 14 • 10 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 15 • 10 years ago

Comment on attachment 8506490 [details] [diff] [review]
patch, r=mats

Beta+
Aurora+

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/6e90cd2aac35
https://hg.mozilla.org/releases/mozilla-beta/rev/f681d39b83ca

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f681d39b83ca

	alejagapatrick10
	Comment 18 • 8 years ago

[Tracking Requested - why for this release]: