1349388 - Various improvements to AutoReferenceLimiter

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Jonathan Watt [:jwatt]]

We should make AutoReferenceLimiter report long reference chains and reference loops to the console.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Make AutoReferenceLimiter report long reference chains and reference loops to the console

Comment 2

[Robert Longson [:longsonr]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Add guard to AutoReferenceLimiter to prevent it being used as a temporary

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Provide AutoReferenceLimiter with a default maximum reference chain length

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 4 - Allow a single instance of AutoReferenceLimiter to guard against both reference loops and long reference chains

Comment 7

[Jonathan Watt [:jwatt]]

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. :)

Comment 8

[Jonathan Watt [:jwatt]]

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.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: part 5 - Assert in AutoReferenceChainGuard's ctor that we didn't fail to break a loop

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Jonathan Watt [:jwatt]]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d439151440a
https://hg.mozilla.org/mozilla-central/rev/cc8f5fa2f481
https://hg.mozilla.org/mozilla-central/rev/6fc806a0dba3
https://hg.mozilla.org/mozilla-central/rev/ce1162be1c86
https://hg.mozilla.org/mozilla-central/rev/49694b5e839d