Changeset 218747 in webkit

Timestamp:

Jun 23, 2017 8:44:30 AM (7 years ago)

Author:

Chris Dumez

Message:

[mac-wk1] requestAnimationFrame callbacks may not get serviced
​https://bugs.webkit.org/show_bug.cgi?id=173628

Reviewed by Simon Fraser.

Source/WebCore:

Page::setIsVisibleInternal() was firing the 'visibilitychange' event
synchronously while in the middle of updating its visibility/activity
state. This allowed the JavaScript to re-enter the method by calling
testRunner.setPageVisibility() / resetPageVisiblity() and we would
end up in an inconsistent state.

No new tests, extended existing test.

• dom/Document.cpp:

(WebCore::Document::visibilityStateChanged):
Do no fire the visibilitychange event synchronously as we are in the
middle of updating the page's activity state. Instead fire the
event asynchronously.

• page/Page.cpp:

(WebCore::Page::setIsVisibleInternal):
Move the calling of Document::visibilityStateChanged() until after we're
done updating the page's visibility state.

• testing/Internals.cpp:

(WebCore::Internals::scriptedAnimationsAreSuspended):

• testing/Internals.h:
• testing/Internals.idl:

Add test infrastructure to check if scripted animations are suspended.

LayoutTests:

Extend layout test coverage.

• fast/events/page-visibility-transition-test-expected.txt:
• fast/events/page-visibility-transition-test.html:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/events/page-visibility-transition-test-expected.txt (modified) (1 diff)
• LayoutTests/fast/events/page-visibility-transition-test.html (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (1 diff)
• Source/WebCore/page/Page.cpp (modified) (2 diffs)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-06-23  Chris Dumez  <cdumez@apple.com>
+
+        [mac-wk1] requestAnimationFrame callbacks may not get serviced
+        https://bugs.webkit.org/show_bug.cgi?id=173628
+
+        Reviewed by Simon Fraser.
+
+        Extend layout test coverage.
+
+        * fast/events/page-visibility-transition-test-expected.txt:
+        * fast/events/page-visibility-transition-test.html:
+
 2017-06-23  Zan Dobersek  <zdobersek@igalia.com>
 

trunk/LayoutTests/fast/events/page-visibility-transition-test-expected.txt

@@ -6 +6 @@
 PASS document.visibilityState is "visible"
 PASS document.hidden is false
+PASS internals.scriptedAnimationsAreSuspended is false
 PASS document.visibilityState is "hidden"
 PASS document.hidden is true
+PASS internals.scriptedAnimationsAreSuspended is true
 PASS document.visibilityState is "hidden"
 PASS document.hidden is true
+PASS internals.scriptedAnimationsAreSuspended is true
 PASS document.visibilityState is "visible"
 PASS document.hidden is false
+PASS internals.scriptedAnimationsAreSuspended is false
 PASS successfullyParsed is true
 

trunk/LayoutTests/fast/events/page-visibility-transition-test.html

@@ -33 +33 @@
     shouldBeEqualToString("document.visibilityState", "visible");
     shouldBeFalse("document.hidden");
+    shouldBeFalse("internals.scriptedAnimationsAreSuspended");
 }
 
@@ -38 +39 @@
     shouldBeEqualToString("document.visibilityState", "hidden");
     shouldBeTrue("document.hidden");
+    shouldBeTrue("internals.scriptedAnimationsAreSuspended");
 }
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-06-23  Chris Dumez  <cdumez@apple.com>
+
+        [mac-wk1] requestAnimationFrame callbacks may not get serviced
+        https://bugs.webkit.org/show_bug.cgi?id=173628
+
+        Reviewed by Simon Fraser.
+
+        Page::setIsVisibleInternal() was firing the 'visibilitychange' event
+        synchronously while in the middle of updating its visibility/activity
+        state. This allowed the JavaScript to re-enter the method by calling
+        testRunner.setPageVisibility() / resetPageVisiblity() and we would
+        end up in an inconsistent state.
+
+        No new tests, extended existing test.
+
+        * dom/Document.cpp:
+        (WebCore::Document::visibilityStateChanged):
+        Do no fire the visibilitychange event synchronously as we are in the
+        middle of updating the page's activity state. Instead fire the
+        event asynchronously.
+
+        * page/Page.cpp:
+        (WebCore::Page::setIsVisibleInternal):
+        Move the calling of Document::visibilityStateChanged() until after we're
+        done updating the page's visibility state.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::scriptedAnimationsAreSuspended):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+        Add test infrastructure to check if scripted animations are suspended.
+
 2017-06-23  Eric Carlson  <eric.carlson@apple.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -1618 +1618 @@
 void Document::visibilityStateChanged()
 {
-    dispatchEvent(Event::create(eventNames().visibilitychangeEvent, false, false));
+    enqueueDocumentEvent(Event::create(eventNames().visibilitychangeEvent, false, false));
     for (auto* client : m_visibilityStateCallbackClients)
         client->visibilityStateChanged();

trunk/Source/WebCore/page/Page.cpp

@@ -1715 +1715 @@
     }
 
+    if (!isVisible) {
+        if (m_settings->hiddenPageCSSAnimationSuspensionEnabled())
+            mainFrame().animation().suspendAnimations();
+
+        setSVGAnimationsState(*this, SVGAnimationsState::Paused);
+
+#if PLATFORM(IOS)
+        suspendDeviceMotionAndOrientationUpdates();
+#endif
+
+#if PLATFORM(MAC)
+        if (MacApplication::isDumpRenderTree())
+            WTFLogAlways("\nPage::setIsVisibleInternal(%s), %p", isVisible ? "true" : "false", this);
+#endif
+        suspendScriptedAnimations();
+
+        if (FrameView* view = mainFrame().view())
+            view->hide();
+    }
+
     Vector<Ref<Document>> documents;
     for (Frame* frame = &m_mainFrame.get(); frame; frame = frame->tree().traverseNext())
@@ -1721 +1741 @@
     for (auto& document : documents)
         document->visibilityStateChanged();
-
-    if (!isVisible) {
-        if (m_settings->hiddenPageCSSAnimationSuspensionEnabled())
-            mainFrame().animation().suspendAnimations();
-
-        setSVGAnimationsState(*this, SVGAnimationsState::Paused);
-
-#if PLATFORM(IOS)
-        suspendDeviceMotionAndOrientationUpdates();
-#endif
-
-#if PLATFORM(MAC)
-        if (MacApplication::isDumpRenderTree())
-            WTFLogAlways("\nPage::setIsVisibleInternal(%s), %p", isVisible ? "true" : "false", this);
-#endif
-        suspendScriptedAnimations();
-
-        if (FrameView* view = mainFrame().view())
-            view->hide();
-    }
 }
 

trunk/Source/WebCore/testing/Internals.cpp

@@ -1164 +1164 @@
 }
 
+bool Internals::scriptedAnimationsAreSuspended() const
+{
+    Document* document = contextDocument();
+    if (!document || !document->page())
+        return true;
+
+    return document->page()->scriptedAnimationsSuspended();
+}
+
 bool Internals::areTimersThrottled() const
 {

trunk/Source/WebCore/testing/Internals.h

@@ -153 +153 @@
     bool isRequestAnimationFrameThrottled() const;
     double requestAnimationFrameInterval() const;
+    bool scriptedAnimationsAreSuspended() const;
     bool areTimersThrottled() const;
 

trunk/Source/WebCore/testing/Internals.idl

@@ -371 +371 @@
     [MayThrowException] void setLowPowerModeEnabled(boolean enabled);
     readonly attribute double requestAnimationFrameInterval;
+    readonly attribute boolean scriptedAnimationsAreSuspended;
 
     // Override the behavior of WebPage::eventThrottlingDelay(), which only affects iOS.