[webkit-changes] [199375] trunk

Tue, 12 Apr 2016 11:34:12 -0700

Title: [199375] trunk
Revision
199375
Author
[email protected]
Date
2016-04-12 11:33:56 -0700 (Tue, 12 Apr 2016)

Log Message

[OS X] Flakey crash after ScrollAnimatorMac destruction
https://bugs.webkit.org/show_bug.cgi?id=156372

Reviewed by Darin Adler.

Source/WebCore:

Previously, we were disabling the mock scrollbars using _javascript_ after
the WebView was created. However, enabling these mock scrollbars can be
triggered with a bit of state inside the WebPreferences object, which
means WebKit clients can change it at any point. DumpRenderTree is doing
this during the document's lifetime.

This means that the creation of the Scrollbar objects saw a non-mock
ScrollbarTheme, but the destruction of the Scrollbar objects saw a mock
ScrollbarTheme. Therefore, the non-mock ScrollbarTheme doesn't get
cleaned up correctly (ScrollAnimatorMac::willRemoveVerticalScrollbar()
returns early because it sees that there is nothing to deregister
due to the ScrollbarTheme being mocked).

This cleanup is necessary because it sets the NSScrollerImp's delegate
to nil before the NSScrollerImpDelegate gets destroyed. Because the
cleanup wasn't happening, the delegate pointer wasn't getting set to
nil, so the pointer was dangling, and AppKit was following it and
crashing.

Because the clients of this bit of state can change it at any time,
it is incorrect to change it in _javascript_. Instead, the client must
manage this bit of state (so the client and the web process are always
in sync). Therefore, the correct way to set this bit of state must be
done in the test runner rather than _javascript_ internals. The mechanism
we have to do that is the <!-- webkit-test-runner --> comment at the
beginning of the test. This patch migrates to this mechanism and removes
the old internals method.

Test: fast/scrolling/rtl-scrollbars-animation-property.html

* page/Settings.cpp:
* testing/Internals.cpp:
(WebCore::Internals::setMockScrollbarsEnabled): Deleted.
* testing/Internals.h:
* testing/Internals.idl:

Tools:

Implement the new <!-- webkit-test-runner --> flag.

* WebKitTestRunner/TestController.cpp:
(WTR::TestController::createWebViewWithOptions):
(WTR::TestController::ensureViewSupportsOptionsForTest):
(WTR::TestController::resetPreferencesToConsistentValues):
(WTR::TestController::resetStateToConsistentValues):
(WTR::updateTestOptionsFromTestHeader):
* WebKitTestRunner/TestController.h:
* WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::invoke):
* WebKitTestRunner/TestOptions.h:
* WebKitTestRunner/mac/PlatformWebViewMac.mm:
(WKR::PlatformWebView::viewSupportsOptions):

LayoutTests:

Migrate to the new mechanism for disabling mock scrollbars in tests.

* fast/scrolling/rtl-scrollbars-animation-property.html:

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (199374 => 199375)


--- trunk/LayoutTests/ChangeLog	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/LayoutTests/ChangeLog	2016-04-12 18:33:56 UTC (rev 199375)
@@ -1,3 +1,14 @@
+2016-04-12  Myles C. Maxfield  <[email protected]>
+
+        [OS X] Flakey crash after ScrollAnimatorMac destruction
+        https://bugs.webkit.org/show_bug.cgi?id=156372
+
+        Reviewed by Darin Adler.
+
+        Migrate to the new mechanism for disabling mock scrollbars in tests.
+
+        * fast/scrolling/rtl-scrollbars-animation-property.html:
+
 2016-04-12  Saam barati  <[email protected]>
 
         We incorrectly parse arrow function expressions

Modified: trunk/LayoutTests/fast/scrolling/rtl-scrollbars-animation-property.html (199374 => 199375)


--- trunk/LayoutTests/fast/scrolling/rtl-scrollbars-animation-property.html	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/LayoutTests/fast/scrolling/rtl-scrollbars-animation-property.html	2016-04-12 18:33:56 UTC (rev 199375)
@@ -1,10 +1,6 @@
-<!DOCTYPE html><!-- webkit-test-runner [ rtlScrollbars=true ] -->
+<!DOCTYPE html><!-- webkit-test-runner [ rtlScrollbars=true useMockScrollbars=false ] -->
 <html>
 <head>
-<script>
-if (window.internals)
-    window.internals.setMockScrollbarsEnabled(false);
-</script>
 </head>
 <body>
 <div style="width: 200px; height: 200px; position: relative; overflow: scroll;">

Modified: trunk/LayoutTests/platform/ios-simulator/fast/scrolling/rtl-scrollbars-animation-property-expected.txt (199374 => 199375)


--- trunk/LayoutTests/platform/ios-simulator/fast/scrolling/rtl-scrollbars-animation-property-expected.txt	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/LayoutTests/platform/ios-simulator/fast/scrolling/rtl-scrollbars-animation-property-expected.txt	2016-04-12 18:33:56 UTC (rev 199375)
@@ -3,9 +3,9 @@
 layer at (0,0) size 800x216
   RenderBlock {HTML} at (0,0) size 800x216
     RenderBody {BODY} at (8,8) size 784x200
-layer at (8,8) size 200x200 scrollHeight 2000
+layer at (8,8) size 200x200 clip at (8,8) size 185x185 scrollHeight 2000
   RenderBlock (relative positioned) {DIV} at (0,0) size 200x200
-layer at (8,8) size 1x2000 backgroundClip at (8,8) size 200x200 clip at (8,8) size 200x200
+layer at (8,8) size 1x2000 backgroundClip at (8,8) size 185x185 clip at (8,8) size 185x185
   RenderBlock (positioned) {DIV} at (0,0) size 1x2000
 layer at (0,0) size 1x2000
   RenderBlock (positioned) {DIV} at (0,0) size 1x2000

Modified: trunk/Source/WebCore/ChangeLog (199374 => 199375)


--- trunk/Source/WebCore/ChangeLog	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Source/WebCore/ChangeLog	2016-04-12 18:33:56 UTC (rev 199375)
@@ -1,3 +1,46 @@
+2016-04-12  Myles C. Maxfield  <[email protected]>
+
+        [OS X] Flakey crash after ScrollAnimatorMac destruction
+        https://bugs.webkit.org/show_bug.cgi?id=156372
+
+        Reviewed by Darin Adler.
+
+        Previously, we were disabling the mock scrollbars using _javascript_ after
+        the WebView was created. However, enabling these mock scrollbars can be
+        triggered with a bit of state inside the WebPreferences object, which
+        means WebKit clients can change it at any point. DumpRenderTree is doing
+        this during the document's lifetime.
+
+        This means that the creation of the Scrollbar objects saw a non-mock
+        ScrollbarTheme, but the destruction of the Scrollbar objects saw a mock
+        ScrollbarTheme. Therefore, the non-mock ScrollbarTheme doesn't get
+        cleaned up correctly (ScrollAnimatorMac::willRemoveVerticalScrollbar()
+        returns early because it sees that there is nothing to deregister
+        due to the ScrollbarTheme being mocked).
+
+        This cleanup is necessary because it sets the NSScrollerImp's delegate
+        to nil before the NSScrollerImpDelegate gets destroyed. Because the
+        cleanup wasn't happening, the delegate pointer wasn't getting set to
+        nil, so the pointer was dangling, and AppKit was following it and
+        crashing.
+
+        Because the clients of this bit of state can change it at any time,
+        it is incorrect to change it in _javascript_. Instead, the client must
+        manage this bit of state (so the client and the web process are always
+        in sync). Therefore, the correct way to set this bit of state must be
+        done in the test runner rather than _javascript_ internals. The mechanism
+        we have to do that is the <!-- webkit-test-runner --> comment at the
+        beginning of the test. This patch migrates to this mechanism and removes
+        the old internals method.
+
+        Test: fast/scrolling/rtl-scrollbars-animation-property.html
+
+        * page/Settings.cpp:
+        * testing/Internals.cpp:
+        (WebCore::Internals::setMockScrollbarsEnabled): Deleted.
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2016-04-12  Darin Adler  <[email protected]>
 
         Remove UsePointersEvenForNonNullableObjectArguments from SVG lists

Modified: trunk/Source/WebCore/page/Settings.cpp (199374 => 199375)


--- trunk/Source/WebCore/page/Settings.cpp	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Source/WebCore/page/Settings.cpp	2016-04-12 18:33:56 UTC (rev 199375)
@@ -626,6 +626,10 @@
         m_page->mainFrame().view()->setScrollingPerformanceLoggingEnabled(enabled);
 }
 
+// It's very important that this setting doesn't change in the middle of a document's lifetime.
+// The Mac port uses this flag when registering and deregistering platform-dependent scrollbar
+// objects. Therefore, if this changes at an unexpected time, deregistration may not happen
+// correctly, which may cause the platform to follow dangling pointers.
 void Settings::setMockScrollbarsEnabled(bool flag)
 {
     gMockScrollbarsEnabled = flag;

Modified: trunk/Source/WebCore/testing/Internals.cpp (199374 => 199375)


--- trunk/Source/WebCore/testing/Internals.cpp	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Source/WebCore/testing/Internals.cpp	2016-04-12 18:33:56 UTC (rev 199375)
@@ -2749,11 +2749,6 @@
     WebCore::Settings::setUsesMockScrollAnimator(enabled);
 }
 
-void Internals::setMockScrollbarsEnabled(bool enabled)
-{
-    WebCore::Settings::setMockScrollbarsEnabled(enabled);
-}
-
 void Internals::forceReload(bool endToEnd)
 {
     frame()->loader().reload(endToEnd);

Modified: trunk/Source/WebCore/testing/Internals.h (199374 => 199375)


--- trunk/Source/WebCore/testing/Internals.h	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Source/WebCore/testing/Internals.h	2016-04-12 18:33:56 UTC (rev 199375)
@@ -351,7 +351,6 @@
 
     void setUsesOverlayScrollbars(bool);
     void setUsesMockScrollAnimator(bool);
-    void setMockScrollbarsEnabled(bool);
 
     String getCurrentCursorInfo(ExceptionCode&);

Modified: trunk/Source/WebCore/testing/Internals.idl (199374 => 199375)


--- trunk/Source/WebCore/testing/Internals.idl	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Source/WebCore/testing/Internals.idl	2016-04-12 18:33:56 UTC (rev 199375)
@@ -354,7 +354,6 @@
 
     void setUsesOverlayScrollbars(boolean enabled);
     void setUsesMockScrollAnimator(boolean enabled);
-    void setMockScrollbarsEnabled(boolean enabled);
 
     void forceReload(boolean endToEnd);

Modified: trunk/Tools/ChangeLog (199374 => 199375)


--- trunk/Tools/ChangeLog	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Tools/ChangeLog	2016-04-12 18:33:56 UTC (rev 199375)
@@ -1,3 +1,25 @@
+2016-04-12  Myles C. Maxfield  <[email protected]>
+
+        [OS X] Flakey crash after ScrollAnimatorMac destruction
+        https://bugs.webkit.org/show_bug.cgi?id=156372
+
+        Reviewed by Darin Adler.
+
+        Implement the new <!-- webkit-test-runner --> flag.
+
+        * WebKitTestRunner/TestController.cpp:
+        (WTR::TestController::createWebViewWithOptions):
+        (WTR::TestController::ensureViewSupportsOptionsForTest):
+        (WTR::TestController::resetPreferencesToConsistentValues):
+        (WTR::TestController::resetStateToConsistentValues):
+        (WTR::updateTestOptionsFromTestHeader):
+        * WebKitTestRunner/TestController.h:
+        * WebKitTestRunner/TestInvocation.cpp:
+        (WTR::TestInvocation::invoke):
+        * WebKitTestRunner/TestOptions.h:
+        * WebKitTestRunner/mac/PlatformWebViewMac.mm:
+        (WKR::PlatformWebView::viewSupportsOptions):
+
 2016-04-12  Tomas Popela  <[email protected]>
 
         Modify the CXXFLAGS in webkitdirs.pm just on architectures where the flags are supported

Modified: trunk/Tools/WebKitTestRunner/TestController.cpp (199374 => 199375)


--- trunk/Tools/WebKitTestRunner/TestController.cpp	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Tools/WebKitTestRunner/TestController.cpp	2016-04-12 18:33:56 UTC (rev 199375)
@@ -489,7 +489,7 @@
 
     // Some preferences (notably mock scroll bars setting) currently cannot be re-applied to an existing view, so we need to set them now.
     // FIXME: Migrate these preferences to WKContextConfigurationRef.
-    resetPreferencesToConsistentValues();
+    resetPreferencesToConsistentValues(options);
 
     platformCreateWebView(configuration.get(), options);
     WKPageUIClientV6 pageUIClient = {
@@ -633,11 +633,11 @@
 
     createWebViewWithOptions(options);
 
-    if (!resetStateToConsistentValues())
+    if (!resetStateToConsistentValues(options))
         TestInvocation::dumpWebProcessUnresponsiveness("<unknown> - TestController::run - Failed to reset state to consistent values\n");
 }
 
-void TestController::resetPreferencesToConsistentValues()
+void TestController::resetPreferencesToConsistentValues(const TestOptions& options)
 {
     // Reset preferences
     WKPreferencesRef preferences = platformPreferences();
@@ -664,7 +664,7 @@
     WKPreferencesSetArtificialPluginInitializationDelayEnabled(preferences, false);
     WKPreferencesSetTabToLinksEnabled(preferences, false);
     WKPreferencesSetInteractiveFormValidationEnabled(preferences, true);
-    WKPreferencesSetMockScrollbarsEnabled(preferences, true);
+    WKPreferencesSetMockScrollbarsEnabled(preferences, options.useMockScrollbars);
 
     static WKStringRef defaultTextEncoding = WKStringCreateWithUTF8CString("ISO-8859-1");
     WKPreferencesSetDefaultTextEncodingName(preferences, defaultTextEncoding);
@@ -704,7 +704,7 @@
     platformResetPreferencesToConsistentValues();
 }
 
-bool TestController::resetStateToConsistentValues()
+bool TestController::resetStateToConsistentValues(const TestOptions& options)
 {
     TemporaryChange<State> changeState(m_state, Resetting);
     m_beforeUnloadReturnValue = true;
@@ -744,7 +744,7 @@
 
     // FIXME: Is this needed? Nothing in TestController changes preferences during tests, and if there is
     // some other code doing this, it should probably be responsible for cleanup too.
-    resetPreferencesToConsistentValues();
+    resetPreferencesToConsistentValues(options);
 
 #if !PLATFORM(COCOA)
     WKTextCheckerContinuousSpellCheckingEnabledStateChanged(true);
@@ -954,6 +954,8 @@
             testOptions.useDataDetection = parseBooleanTestHeaderValue(value);
         if (key == "rtlScrollbars")
             testOptions.useRTLScrollbars = parseBooleanTestHeaderValue(value);
+        if (key == "useMockScrollbars")
+            testOptions.useMockScrollbars = parseBooleanTestHeaderValue(value);
         pairStart = pairEnd + 1;
     }
 }

Modified: trunk/Tools/WebKitTestRunner/TestController.h (199374 => 199375)


--- trunk/Tools/WebKitTestRunner/TestController.h	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Tools/WebKitTestRunner/TestController.h	2016-04-12 18:33:56 UTC (rev 199375)
@@ -111,8 +111,8 @@
     // Page Visibility.
     void setHidden(bool);
 
-    bool resetStateToConsistentValues();
-    void resetPreferencesToConsistentValues();
+    bool resetStateToConsistentValues(const TestOptions&);
+    void resetPreferencesToConsistentValues(const TestOptions&);
 
     void terminateWebContentProcess();
     void reattachPageToWebProcess();

Modified: trunk/Tools/WebKitTestRunner/TestInvocation.cpp (199374 => 199375)


--- trunk/Tools/WebKitTestRunner/TestInvocation.cpp	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Tools/WebKitTestRunner/TestInvocation.cpp	2016-04-12 18:33:56 UTC (rev 199375)
@@ -171,7 +171,7 @@
 
     if (m_webProcessIsUnresponsive)
         dumpWebProcessUnresponsiveness();
-    else if (!TestController::singleton().resetStateToConsistentValues()) {
+    else if (!TestController::singleton().resetStateToConsistentValues(m_options)) {
         // The process froze while loading about:blank, let's start a fresh one.
         // It would be nice to report that the previous test froze after dumping results, but we have no way to do that.
         TestController::singleton().terminateWebContentProcess();

Modified: trunk/Tools/WebKitTestRunner/TestOptions.h (199374 => 199375)


--- trunk/Tools/WebKitTestRunner/TestOptions.h	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Tools/WebKitTestRunner/TestOptions.h	2016-04-12 18:33:56 UTC (rev 199375)
@@ -41,6 +41,7 @@
     bool isHiDPITest { false };
     bool useDataDetection { false };
     bool useRTLScrollbars { false };
+    bool useMockScrollbars { true };
 
     Vector<String> overrideLanguages;

Modified: trunk/Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm (199374 => 199375)


--- trunk/Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm	2016-04-12 17:55:44 UTC (rev 199374)
+++ trunk/Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm	2016-04-12 18:33:56 UTC (rev 199375)
@@ -233,7 +233,7 @@
 
 bool PlatformWebView::viewSupportsOptions(const TestOptions& options) const
 {
-    if (m_options.useThreadedScrolling != options.useThreadedScrolling || m_options.overrideLanguages != options.overrideLanguages || m_options.useRTLScrollbars != options.useRTLScrollbars)
+    if (m_options.useThreadedScrolling != options.useThreadedScrolling || m_options.overrideLanguages != options.overrideLanguages || m_options.useRTLScrollbars != options.useRTLScrollbars || m_options.useMockScrollbars != options.useMockScrollbars)
         return false;
 
     return true;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to