Make named CounterStyle not refcounted

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

53 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
It doesn't really make much sense to have refcounted arena-allocated objects...

Looking at existing usage, I think we can do this change:
1. in CounterStyleManager::NotifyRuleChanged(), collect all removed objects into a list,
2. destroy objects in the list after next restyle
3. destroy all remaining objects in the cache table when disconnecting

Currently, there are two places which can own a CounterStyle outside CounterStyleManager, one is nsStyleList, the other is nsCounterUseNode.

The pointer in nsCounterUseNode objects will be cleared immediately after CounterStyleManager::NotifyRuleChange() is invoked.

And nsStyleList should be destroyed after a restyle... even if it is not, after restyle, frames should no longer be using the old style data, and nsComputedDOMStyle should update its reference before querying the data anyway, so nothing should try to access the CounterStyle object anymore.

I think that's safe... even if it is not that safe, memory poisoning by presshell arena should protect us to some extent.
(Assignee)

Comment 1

2 years ago
Anonymous counter style should still be refcounted because it is not managed by CounterStyleManager.
(Assignee)

Updated

2 years ago
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8866641 [details]
Bug 1363699 part 1 - Make named CounterStyle objects not refcounted.

https://reviewboard.mozilla.org/r/138210/#review142280

OK, just got a little bit into this, but may as well publish the comments I have so far.  Hopefully I'll continue on Tuesday, although I'm not sure...  (If you want someone faster, I'm fine with you finding another reviewer.)

::: layout/style/CounterStyleManager.h:143
(Diff revision 1)
>    bool mSingleString;
>    uint8_t mSystem;
>    nsTArray<nsString> mSymbols;
>  };
>  
> +// A smart pointer to CounterStyle. It either strongly owns an anonymous

s/strongly owns/owns a reference to/.

(I'd interpret "strongly owns" as UniquePtr rather than RefPtr, although I guess it's ambiguous.)

::: layout/style/CounterStyleManager.cpp:1972
(Diff revision 1)
> +    style->~CustomCounterStyle();
> +    shell->FreeByObjectID(eArenaObjectID_CustomCounterStyle, style);

Please keep this code in a Destroy method on CustomCounterStyle rather than having it written out here.
(Assignee)

Comment 6

2 years ago
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #5)
> Comment on attachment 8866641 [details]
> Bug 1363699 part 1 - Make named CounterStyle objects not refcounted.
> 
> https://reviewboard.mozilla.org/r/138210/#review142280
> 
> OK, just got a little bit into this, but may as well publish the comments I
> have so far.  Hopefully I'll continue on Tuesday, although I'm not sure... 
> (If you want someone faster, I'm fine with you finding another reviewer.)

Your next Tuesday is probably my next Wednesday... which sounds like it would block other things for too long... so I'll probably try heycam then. Thanks for telling me that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8866641 [details]
Bug 1363699 part 1 - Make named CounterStyle objects not refcounted.

https://reviewboard.mozilla.org/r/138210/#review142358

::: layout/style/CounterStyleManager.h:172
(Diff revision 2)
> +  CounterStylePtr& operator=(AnonymousCounterStyle* aCounterStyle)
> +  {
> +    MOZ_ASSERT(aCounterStyle);

It probably makes sense to handle aCounterStyle being null, because we could invoke this operator= with code like:

  CounterStylePtr p;
  AnonymousCounterStyle* cs = nullptr;
  p = cs;

::: layout/style/CounterStyleManager.h:180
(Diff revision 2)
> +  CounterStylePtr& operator=(CounterStyle* aCounterStyle)
> +  {
> +    MOZ_ASSERT(aCounterStyle);

Here too.

::: layout/style/CounterStyleManager.cpp:1983
(Diff revision 2)
>  }
>  
>  void
> +CounterStyleManager::DestroyCounterStyle(CounterStyle* aCounterStyle)
> +{
> +  nsIPresShell* shell = PresContext()->PresShell();

Don't need this?

::: layout/style/CounterStyleManager.cpp:2102
(Diff revision 2)
>            // manual ResetDependentData() call. (This only really matters if
>            // something else is holding a reference and keeping it alive.)

Is this comment out of date, since we're assuming that nothing can keep it alive after the restyle?

::: layout/style/ServoBindings.cpp:1125
(Diff revision 2)
>  Gecko_SetListStyleType(nsStyleList* style_struct, uint32_t type)
>  {
> -  // Builtin counter styles are static and use no-op refcounting, and thus are
> +  style_struct->mCounterStyle = CounterStyleManager::GetBuiltinStyle(type);

Same comment as before: should prefer to use Gecko-style argument naming in this cpp file.
Attachment #8866641 - Flags: review?(cam) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8866642 [details]
Bug 1363699 part 2 - Make AnonymousCounterStyle use thread-safe refcount.

https://reviewboard.mozilla.org/r/138242/#review142364
Attachment #8866642 - Flags: review?(cam) → review+
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8866641 [details]
Bug 1363699 part 1 - Make named CounterStyle objects not refcounted.

https://reviewboard.mozilla.org/r/138210/#review142358

> Is this comment out of date, since we're assuming that nothing can keep it alive after the restyle?

Yeah, we probably don't need this anymore, given that all removed styles would be destroyed at the same time, so it doesn't make much sense to clear references between them.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95d6b2b7ed83
part 1 - Make named CounterStyle objects not refcounted. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f21712b81ee6
part 2 - Make AnonymousCounterStyle use thread-safe refcount. r=heycam

Comment 15

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3e1ae3760e1
followup - Update hazard analysis whitelist.

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/95d6b2b7ed83
https://hg.mozilla.org/mozilla-central/rev/f21712b81ee6
https://hg.mozilla.org/mozilla-central/rev/d3e1ae3760e1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.