Various improvements to AutoReferenceLimiter

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

a year ago
We should make AutoReferenceLimiter report long reference chains and reference loops to the console.
(Assignee)

Comment 1

a year ago
Created attachment 8849757 [details] [diff] [review]
part 1 - Make AutoReferenceLimiter report long reference chains and reference loops to the console
Assignee: nobody → jwatt
Attachment #8849757 - Flags: review?(longsonr)
(Assignee)

Updated

a year ago
Attachment #8849757 - Attachment description: patch → part 1 - Make AutoReferenceLimiter report long reference chains and reference loops to the console
(Assignee)

Updated

a year ago
Summary: Make AutoReferenceLimiter report long reference chains and reference loops to the console → Various improvements to AutoReferenceLimiter
Comment on attachment 8849757 [details] [diff] [review]
part 1 - Make AutoReferenceLimiter report long reference chains and reference loops to the console

Seems fine. Do we not do this for patterns, masks and filters though? You should raise a follow up to track doing that.
Attachment #8849757 - Flags: review?(longsonr) → review+
(Assignee)

Comment 3

a year ago
Yeah, I have patches to do (I think) all of those. I'm just in the process of unbitrotting them and then making them compatible with the AutoReferenceLimiter changes I'll add here.
(Assignee)

Comment 4

a year ago
Created attachment 8849784 [details] [diff] [review]
part 2 - Add guard to AutoReferenceLimiter to prevent it being used as a temporary
Attachment #8849784 - Flags: review?(longsonr)
Attachment #8849784 - Flags: review?(longsonr) → review+
(Assignee)

Comment 5

a year ago
Created attachment 8849815 [details] [diff] [review]
part 3 - Provide AutoReferenceLimiter with a default maximum reference chain length
Attachment #8849815 - Flags: review?(longsonr)
(Assignee)

Comment 6

a year ago
Created attachment 8849816 [details] [diff] [review]
part 4 - Allow a single instance of AutoReferenceLimiter to guard against both reference loops and long reference chains
Attachment #8849816 - Flags: review?(longsonr)
(Assignee)

Comment 7

a year ago
Comment on attachment 8849815 [details] [diff] [review]
part 3 - Provide AutoReferenceLimiter with a default maximum reference chain length

Review of attachment 8849815 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/AutoReferenceLimiter.h
@@ +62,5 @@
>   *   }
>   */
>  class MOZ_RAII AutoReferenceLimiter
>  {
> +  static const int16_t sDefaultMaxChainLength = 10; // arbitrary number

This may seem a little aggressively low, but I think the amount of content that will be affected will be tiny - and if it's not I'd like to know about it. :)
(Assignee)

Comment 8

a year ago
Comment on attachment 8849816 [details] [diff] [review]
part 4 - Allow a single instance of AutoReferenceLimiter to guard against both reference loops and long reference chains

Review of attachment 8849816 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/AutoReferenceChainGuard.h
@@ +140,5 @@
>      nsAutoString tag;
>      mFrame->GetContent()->AsElement()->GetTagName(tag);
>      const char16_t* params[] = { tag.get() };
>      auto doc = mFrame->GetContent()->OwnerDoc();
> +    auto warning = mFrameInUse ?

This should be *mFrameInUse - fixed locally.
(Assignee)

Comment 9

a year ago
Created attachment 8849818 [details] [diff] [review]
part 5 - Assert in AutoReferenceChainGuard's ctor that we didn't fail to break a loop
Attachment #8849818 - Flags: review?(longsonr)
Attachment #8849815 - Flags: review?(longsonr) → review+
Attachment #8849816 - Flags: review?(longsonr) → review+
Attachment #8849818 - Flags: review?(longsonr) → review+

Comment 10

a year ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d439151440a
part 1 - Make AutoReferenceLimiter report long reference chains and reference loops to the console. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8f5fa2f481
part 2 - Add guard to AutoReferenceLimiter to prevent it being used as a temporary. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc806a0dba3
part 3 - Provide AutoReferenceLimiter with a default maximum reference chain length. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1162be1c86
part 4 - Allow a single instance of AutoReferenceLimiter to guard against both reference loops and long reference chains. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/49694b5e839d
part 5 - Assert in AutoReferenceChainGuard's ctor that we didn't fail to break a loop. r=longsonr
(Assignee)

Comment 11

a year ago
(In reply to Robert Longson from comment #2)
> Seems fine. Do we not do this for patterns, masks and filters though? You
> should raise a follow up to track doing that.

Bug 1349477.
Blocks: 1349477
Blocks: 348864
You need to log in before you can comment on or make changes to this bug.