Closed Bug 1363699 Opened 7 years ago Closed 7 years ago

Make named CounterStyle not refcounted

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

Details

Attachments

(2 files)

Bug 1363699 part 1 - Make named CounterStyle objects not refcounted.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363699 part 2 - Make AnonymousCounterStyle use thread-safe refcount.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
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.
Anonymous counter style should still be refcounted because it is not managed by CounterStyleManager.
Assignee: nobody → xidorn+moz
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.
(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 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 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+
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.
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
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3e1ae3760e1
followup - Update hazard analysis whitelist.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: