Page overlay tests sometimes crash under MockPageOverlayClient::uninstallAllOverlays()
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Apr 2016 04:54:02 +0000 (04:54 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Apr 2016 04:54:02 +0000 (04:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=156080
rdar://problem/24922183

Reviewed by Alex Christensen.

Make MockPageOverlayClient::m_overlays reference the overlays. There is no reference
cycle because they are all removed between test page loads.

* testing/MockPageOverlayClient.cpp:
(WebCore::MockPageOverlayClient::uninstallAllOverlays):
(WebCore::MockPageOverlayClient::pageOverlayDestroyed):
* testing/MockPageOverlayClient.h:

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

Source/WebCore/ChangeLog
Source/WebCore/testing/MockPageOverlayClient.cpp
Source/WebCore/testing/MockPageOverlayClient.h

index d89a6e0..5b9d214 100644 (file)
@@ -1,3 +1,19 @@
+2016-03-31  Alexey Proskuryakov  <ap@apple.com>
+
+        Page overlay tests sometimes crash under MockPageOverlayClient::uninstallAllOverlays()
+        https://bugs.webkit.org/show_bug.cgi?id=156080
+        rdar://problem/24922183
+
+        Reviewed by Alex Christensen.
+
+        Make MockPageOverlayClient::m_overlays reference the overlays. There is no reference
+        cycle because they are all removed between test page loads.
+
+        * testing/MockPageOverlayClient.cpp:
+        (WebCore::MockPageOverlayClient::uninstallAllOverlays):
+        (WebCore::MockPageOverlayClient::pageOverlayDestroyed):
+        * testing/MockPageOverlayClient.h:
+
 2016-03-31  Daniel Bates  <dabates@apple.com>
 
         REGRESSION (r197724): <object>/<embed> with no URL does not match source *
index 03bb9ea..d2f9ee2 100644 (file)
@@ -61,7 +61,7 @@ Ref<MockPageOverlay> MockPageOverlayClient::installOverlay(MainFrame& mainFrame,
 void MockPageOverlayClient::uninstallAllOverlays()
 {
     while (!m_overlays.isEmpty()) {
-        MockPageOverlay* mockOverlay = m_overlays.takeAny();
+        RefPtr<MockPageOverlay> mockOverlay = m_overlays.takeAny();
         PageOverlayController* overlayController = mockOverlay->overlay()->controller();
         ASSERT(overlayController);
         overlayController->uninstallPageOverlay(mockOverlay->overlay(), PageOverlay::FadeMode::DoNotFade);
@@ -75,6 +75,9 @@ String MockPageOverlayClient::layerTreeAsText(MainFrame& mainFrame)
 
 void MockPageOverlayClient::pageOverlayDestroyed(PageOverlay& overlay)
 {
+    // FIXME: This is dead code, nothing ever calls this function. It's not clear to me what the intention was,
+    // since MockPageOverlayClient has references to MockPageOverlays, not to PageOverlays.
+    // Also, iterating over a set while modifying it is not good.
     for (auto& mockOverlay : m_overlays) {
         if (mockOverlay->overlay() == &overlay) {
             m_overlays.remove(mockOverlay);
index b8b3e04..cb39f8a 100644 (file)
@@ -60,7 +60,7 @@ private:
     bool copyAccessibilityAttributeBoolValueForPoint(PageOverlay&, String /* attribute */, FloatPoint, bool&) override;
     Vector<String> copyAccessibilityAttributeNames(PageOverlay&, bool /* parameterizedNames */) override;
 
-    HashSet<MockPageOverlay*> m_overlays;
+    HashSet<RefPtr<MockPageOverlay>> m_overlays;
 };
 
 }