Lots of sticky tests failing in WK2
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Dec 2012 19:30:59 +0000 (19:30 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Dec 2012 19:30:59 +0000 (19:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=105464

Source/WebCore:

Reviewed by Brady Eidson.

Fixing this bug altered the timing of InternalSettings::Backup()
and restoreTo(), such that 'canStartMedia' on Page would get saved as false,
and then restored as false, which breaks all media tests.

'canStartMedia' is set to false initially by the WebPage constructor, and is also
set to false when the WKView leaves the window, so treating it like other Settings
that can be saved and restored is unreliable, as seen here. It's easier and simpler
to just always reset it to 'true' between tests.

* testing/InternalSettings.cpp:
(WebCore::InternalSettings::Backup::Backup):
(WebCore::InternalSettings::Backup::restoreTo):
(WebCore::InternalSettings::InternalSettings):
(WebCore::InternalSettings::reset):
* testing/InternalSettings.h:
(Backup):

Tools:

Reviewed by Beth Dakin.

WebKitTestRunner had a race between snapshotting in the UI process,
and resettting after the test in the web process. InjectedBundle::done()
was a bad place to call page()->resetAfterTest(), because of this race;
it could reset the scroll position before the UI snapshot had been obtained.

Fix by moving the call to page()->resetAfterTest() into didReceiveMessage(),
for the "Reset" message which will come in before the next test.

* WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
(WTR::InjectedBundle::didReceiveMessage):
(WTR::InjectedBundle::done):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@138278 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/testing/InternalSettings.cpp
Source/WebCore/testing/InternalSettings.h
Tools/ChangeLog
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp

index 21f3770..161d835 100644 (file)
@@ -1,3 +1,27 @@
+2012-12-19  Simon Fraser  <simon.fraser@apple.com>
+
+        Lots of sticky tests failing in WK2
+        https://bugs.webkit.org/show_bug.cgi?id=105464
+
+        Reviewed by Brady Eidson.
+
+        Fixing this bug altered the timing of InternalSettings::Backup()
+        and restoreTo(), such that 'canStartMedia' on Page would get saved as false,
+        and then restored as false, which breaks all media tests.
+        
+        'canStartMedia' is set to false initially by the WebPage constructor, and is also
+        set to false when the WKView leaves the window, so treating it like other Settings
+        that can be saved and restored is unreliable, as seen here. It's easier and simpler
+        to just always reset it to 'true' between tests.
+
+        * testing/InternalSettings.cpp:
+        (WebCore::InternalSettings::Backup::Backup):
+        (WebCore::InternalSettings::Backup::restoreTo):
+        (WebCore::InternalSettings::InternalSettings):
+        (WebCore::InternalSettings::reset):
+        * testing/InternalSettings.h:
+        (Backup):
+
 2012-12-20  Erik Arvidsson  <arv@chromium.org>
 
         REGRESSION(r138263): Don't use fastGetAttribute for HTMLNames::classAttr because it breaks on SVGElement
index e0b89df..7e91ed9 100644 (file)
@@ -61,7 +61,7 @@
 
 namespace WebCore {
 
-InternalSettings::Backup::Backup(Page* page, Settings* settings)
+InternalSettings::Backup::Backup(Settings* settings)
     : m_originalPasswordEchoDurationInSeconds(settings->passwordEchoDurationInSeconds())
     , m_originalPasswordEchoEnabled(settings->passwordEchoEnabled())
     , m_originalFixedElementsLayoutRelativeToFrame(settings->fixedElementsLayoutRelativeToFrame())
@@ -91,7 +91,6 @@ InternalSettings::Backup::Backup(Page* page, Settings* settings)
 #if ENABLE(DIALOG_ELEMENT)
     , m_originalDialogElementEnabled(RuntimeEnabledFeatures::dialogElementEnabled())
 #endif
-    , m_canStartMedia(page->canStartMedia())
     , m_originalForceCompositingMode(settings->forceCompositingMode())
     , m_originalCompositingForFixedPositionEnabled(settings->acceleratedCompositingForFixedPositionEnabled())
     , m_originalCompositingForScrollableFramesEnabled(settings->acceleratedCompositingForScrollableFramesEnabled())
@@ -108,7 +107,7 @@ InternalSettings::Backup::Backup(Page* page, Settings* settings)
 }
 
 
-void InternalSettings::Backup::restoreTo(Page* page, Settings* settings)
+void InternalSettings::Backup::restoreTo(Settings* settings)
 {
     settings->setPasswordEchoDurationInSeconds(m_originalPasswordEchoDurationInSeconds);
     settings->setPasswordEchoEnabled(m_originalPasswordEchoEnabled);
@@ -139,7 +138,6 @@ void InternalSettings::Backup::restoreTo(Page* page, Settings* settings)
 #if ENABLE(DIALOG_ELEMENT)
     RuntimeEnabledFeatures::setDialogElementEnabled(m_originalDialogElementEnabled);
 #endif
-    page->setCanStartMedia(m_canStartMedia);
     settings->setForceCompositingMode(m_originalForceCompositingMode);
     settings->setAcceleratedCompositingForFixedPositionEnabled(m_originalCompositingForFixedPositionEnabled);
     settings->setAcceleratedCompositingForScrollableFramesEnabled(m_originalCompositingForScrollableFramesEnabled);
@@ -168,16 +166,17 @@ InternalSettings::~InternalSettings()
 
 InternalSettings::InternalSettings(Page* page)
     : m_page(page)
-    , m_backup(page, page->settings())
+    , m_backup(page->settings())
 {
 }
 
 void InternalSettings::reset()
 {
     page()->setPageScaleFactor(1, IntPoint(0, 0));
+    page()->setCanStartMedia(true);
 
-    m_backup.restoreTo(page(), settings());
-    m_backup = Backup(page(), settings());
+    m_backup.restoreTo(settings());
+    m_backup = Backup(settings());
 }
 
 Settings* InternalSettings::settings() const
index 1fc7f2d..31a1e56 100644 (file)
@@ -46,8 +46,8 @@ class InternalSettings : public RefCountedSupplement<Page, InternalSettings> {
 public:
     class Backup {
     public:
-        Backup(Page*, Settings*);
-        void restoreTo(Page*, Settings*);
+        Backup(Settings*);
+        void restoreTo(Settings*);
 
         double m_originalPasswordEchoDurationInSeconds;
         bool m_originalPasswordEchoEnabled;
@@ -78,7 +78,6 @@ public:
 #if ENABLE(DIALOG_ELEMENT)
         bool m_originalDialogElementEnabled;
 #endif
-        bool m_canStartMedia;
         bool m_originalForceCompositingMode;
         bool m_originalCompositingForFixedPositionEnabled;
         bool m_originalCompositingForScrollableFramesEnabled;
index 270f37d..b742e17 100644 (file)
@@ -1,3 +1,22 @@
+2012-12-19  Simon Fraser  <simon.fraser@apple.com>
+
+        Lots of sticky tests failing in WK2
+        https://bugs.webkit.org/show_bug.cgi?id=105464
+
+        Reviewed by Beth Dakin.
+
+        WebKitTestRunner had a race between snapshotting in the UI process,
+        and resettting after the test in the web process. InjectedBundle::done()
+        was a bad place to call page()->resetAfterTest(), because of this race;
+        it could reset the scroll position before the UI snapshot had been obtained.
+        
+        Fix by moving the call to page()->resetAfterTest() into didReceiveMessage(),
+        for the "Reset" message which will come in before the next test.
+
+        * WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
+        (WTR::InjectedBundle::didReceiveMessage):
+        (WTR::InjectedBundle::done):
+
 2012-12-19  Filip Pizlo  <fpizlo@apple.com>
 
         DFG speculation checks that take JumpList should consolidate OSRExits
index 9d30ad9..c7fe049 100644 (file)
@@ -176,6 +176,8 @@ void InjectedBundle::didReceiveMessage(WKStringRef messageName, WKTypeRef messag
         resetLocalSettings();
         m_testRunner->removeAllWebNotificationPermissions();
 
+        page()->resetAfterTest();
+
         return;
     }
     if (WKStringIsEqualToUTF8CString(messageName, "CallAddChromeInputFieldCallback")) {
@@ -301,8 +303,6 @@ void InjectedBundle::done()
 
     closeOtherPages();
 
-    page()->resetAfterTest();
-
     m_state = Idle;
 }