Bug 170043

Summary: Animated SVG images are not paused in pages loaded in the background
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: SVGAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, buildbot, kling, koivisto, rniwa, sabouhallawa, simon.fraser, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=172183
Bug Depends on: 170155    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch none

Description Chris Dumez 2017-03-23 19:58:42 PDT
SMIL animations inside SVG image do not get paused in background tabs.

Example:
https://pr.gg/svgExamples/smilAnimationImage.html (first animation)

The issue is that SVGImage has its own Page object (SVGImage::m_page) and this page contains the SMIL animation (not the main page). When the tab goes into the background, the main Page knows it is no longer visible and it pauses its SVG animations. However, the Page inside the SVGImage does not currently know it is no longer visible and its SVG animations keep going.

I saw this issue on Yahoo News and reduced it.
Comment 1 Radar WebKit Bug Importer 2017-03-23 19:59:13 PDT
<rdar://problem/31234412>
Comment 2 Chris Dumez 2017-03-23 20:13:18 PDT
I think this is the image causing trouble on Yahoo News:
https://s.yimg.com/os/yc/pt/icons/fuji-loader-v2-blue.0.svg

Seems to be used in a CSS background property.
Comment 3 Chris Dumez 2017-03-29 10:48:26 PDT
Created attachment 305758 [details]
Patch
Comment 4 Build Bot 2017-03-29 11:56:01 PDT
Comment on attachment 305758 [details]
Patch

Attachment 305758 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3434905

New failing tests:
svg/animations/animations-paused-in-background-page-iframe.html
svg/animations/animations-paused-in-background-page.html
Comment 5 Build Bot 2017-03-29 11:56:05 PDT
Created attachment 305765 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-03-29 12:03:57 PDT
Comment on attachment 305758 [details]
Patch

Attachment 305758 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3434904

New failing tests:
svg/animations/animations-paused-in-background-page-iframe.html
svg/animations/animations-paused-in-background-page.html
Comment 7 Build Bot 2017-03-29 12:04:01 PDT
Created attachment 305768 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-03-29 12:28:53 PDT
Comment on attachment 305758 [details]
Patch

Attachment 305758 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3434969

New failing tests:
svg/animations/animations-paused-in-background-page-iframe.html
svg/animations/animations-paused-in-background-page.html
Comment 9 Build Bot 2017-03-29 12:28:58 PDT
Created attachment 305770 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 10 Chris Dumez 2017-03-29 12:34:28 PDT
Created attachment 305771 [details]
Patch
Comment 11 Chris Dumez 2017-03-29 13:54:26 PDT
Comment on attachment 305771 [details]
Patch

Clearing flags on attachment: 305771

Committed r214561: <http://trac.webkit.org/changeset/214561>
Comment 12 Chris Dumez 2017-03-29 13:54:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Said Abou-Hallawa 2017-05-16 12:48:42 PDT
Comment on attachment 305771 [details]
Patch

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

> LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> +Setting page visibility to hidden
> +PASS internals.isImageAnimating(testImage) became false
> +Setting page visibility to visible
> +PASS internals.isImageAnimating(testImage) became false

These expected results do not seem to be correct. The image is not animating regardless whether the page is visible or not.

> LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> +    if (window.testRunner)
> +        testRunner.setPageVisibility('visible');
> +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);

How can this happen? The page visibility is changed to 'visible' but the image is still not animating?
Comment 14 Chris Dumez 2017-05-16 12:50:21 PDT
(In reply to Said Abou-Hallawa from comment #13)
> Comment on attachment 305771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> 
> > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > +Setting page visibility to hidden
> > +PASS internals.isImageAnimating(testImage) became false
> > +Setting page visibility to visible
> > +PASS internals.isImageAnimating(testImage) became false
> 
> These expected results do not seem to be correct. The image is not animating
> regardless whether the page is visible or not.
> 
> > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > +    if (window.testRunner)
> > +        testRunner.setPageVisibility('visible');
> > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> 
> How can this happen? The page visibility is changed to 'visible' but the
> image is still not animating?

This was already fixed in http://trac.webkit.org/r214626.
Comment 15 Said Abou-Hallawa 2017-05-16 13:11:25 PDT
(In reply to Chris Dumez from comment #14)
> (In reply to Said Abou-Hallawa from comment #13)
> > Comment on attachment 305771 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> > 
> > > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > > +Setting page visibility to hidden
> > > +PASS internals.isImageAnimating(testImage) became false
> > > +Setting page visibility to visible
> > > +PASS internals.isImageAnimating(testImage) became false
> > 
> > These expected results do not seem to be correct. The image is not animating
> > regardless whether the page is visible or not.
> > 
> > > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > > +    if (window.testRunner)
> > > +        testRunner.setPageVisibility('visible');
> > > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> > 
> > How can this happen? The page visibility is changed to 'visible' but the
> > image is still not animating?
> 
> This was already fixed in http://trac.webkit.org/r214626.

How this test was passing sometimes? In http://trac.webkit.org/r214626, you just switched the expectation from false to true. Will this new change make the test flaky but because of the opposite reason?
Comment 16 Said Abou-Hallawa 2017-05-16 13:19:29 PDT
(In reply to Said Abou-Hallawa from comment #15)
> (In reply to Chris Dumez from comment #14)
> > (In reply to Said Abou-Hallawa from comment #13)
> > > Comment on attachment 305771 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > > > +Setting page visibility to hidden
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > > +Setting page visibility to visible
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > 
> > > These expected results do not seem to be correct. The image is not animating
> > > regardless whether the page is visible or not.
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > > > +    if (window.testRunner)
> > > > +        testRunner.setPageVisibility('visible');
> > > > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> > > 
> > > How can this happen? The page visibility is changed to 'visible' but the
> > > image is still not animating?
> > 
> > This was already fixed in http://trac.webkit.org/r214626.
> 
> How this test was passing sometimes? In http://trac.webkit.org/r214626, you
> just switched the expectation from false to true. Will this new change make
> the test flaky but because of the opposite reason?

Never mind I just realized http://trac.webkit.org/r214626 is not the fix of the flatness of https://bugs.webkit.org/show_bug.cgi?id=172183 and it was landed 7 weeks ago.
Comment 17 Chris Dumez 2017-05-16 13:20:34 PDT
(In reply to Said Abou-Hallawa from comment #15)
> (In reply to Chris Dumez from comment #14)
> > (In reply to Said Abou-Hallawa from comment #13)
> > > Comment on attachment 305771 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > > > +Setting page visibility to hidden
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > > +Setting page visibility to visible
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > 
> > > These expected results do not seem to be correct. The image is not animating
> > > regardless whether the page is visible or not.
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > > > +    if (window.testRunner)
> > > > +        testRunner.setPageVisibility('visible');
> > > > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> > > 
> > > How can this happen? The page visibility is changed to 'visible' but the
> > > image is still not animating?
> > 
> > This was already fixed in http://trac.webkit.org/r214626.
> 
> How this test was passing sometimes? In http://trac.webkit.org/r214626, you
> just switched the expectation from false to true. Will this new change make
> the test flaky but because of the opposite reason?

There is a small delay between the point the visibility is set and the image actually starts / stops animating.

Previously, the test was sometimes passing because:
1. The page is initially hidden
2. We ask for the page to become visible
3. We poll until the image stops animating

Step 3 will sometimes find that the image is not animating because the page visibility has not been taken into consideration yet.

Since the page is initially hidden and then becomes visible, then the image's state will go from Non-animating to Animating.

If in step 3, we check that the image starts animating (Which is what we really wanted to test and what I did in the follow-up patch), then it will NOT be flaky in the other direction.

The image goes from Non-animating to Animating, and once it is animating, it keeps animating.