Bug 227720 - Make sure SVG SMIL animations do not run in processes without any pages
Summary: Make sure SVG SMIL animations do not run in processes without any pages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-06 14:18 PDT by Chris Dumez
Modified: 2022-01-28 12:12 PST (History)
14 users (show)

See Also:


Attachments
Patch (5.97 KB, patch)
2021-07-06 14:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.00 KB, patch)
2021-07-08 13:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-07-06 14:18:33 PDT
Make sure SVG SMIL animations do not run in processes without any pages.

We have seen traces / logging indicating that SVG SMIL animations may run in cached WebProcesses, thus using CPU unnecessarily. Cached WebProcesses have no WebPage so this patch adds assertions to catch cases where those animations run when there is no Page (and early returns in release to avoid doing unnecessary work).
Comment 1 Chris Dumez 2021-07-06 14:20:28 PDT
Created attachment 432971 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-07-07 20:08:40 PDT
Comment on attachment 432971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432971&action=review

> Source/WebCore/ChangeLog:8
> +        Make sure SVG SMIL animations do not run in processes without any pages.

Perhaps we should instead ensure that SMIL animations don't run in cached SVGImages (or those that don't have a connection to a live Page)? The approach in this patch seems a bit indirect.

> Source/WebCore/ChangeLog:10
> +        We have seen traces / logging indicating that SVG SMIL animations may run in cached WebProcesses, thus

Would be nice if I could find them via a radar chain referenced from this changelog.
Comment 3 Chris Dumez 2021-07-08 12:59:06 PDT
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 432971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432971&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Make sure SVG SMIL animations do not run in processes without any pages.
> 
> Perhaps we should instead ensure that SMIL animations don't run in cached
> SVGImages (or those that don't have a connection to a live Page)? The
> approach in this patch seems a bit indirect.

I made the change to SMILTimeContainer because this is the code we see running in the traces when the process is cached (no WebPage), on a recurring timer. CachedImage already has logic to stop animations when it no longer has any clients.
I don't know where the logic bug is but adding the assertions in SMILTimeContainer seems to me like the surest way to find it?

> > Source/WebCore/ChangeLog:10
> > +        We have seen traces / logging indicating that SVG SMIL animations may run in cached WebProcesses, thus
> 
> Would be nice if I could find them via a radar chain referenced from this
> changelog.

Oh indeed, this is a fix for <rdar://79625708>. I'll update the changelog.
Comment 4 Chris Dumez 2021-07-08 12:59:17 PDT
<rdar://79625708>
Comment 5 Chris Dumez 2021-07-08 13:00:52 PDT
Created attachment 433158 [details]
Patch
Comment 6 EWS 2021-07-08 17:12:37 PDT
Committed r279768 (239538@main): <https://commits.webkit.org/239538@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433158 [details].