[WPE][GTK] WaylandCompositor fails to properly remove surface from its page map
authormcatanzaro@igalia.com <mcatanzaro@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Aug 2018 15:17:55 +0000 (15:17 +0000)
committermcatanzaro@igalia.com <mcatanzaro@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Aug 2018 15:17:55 +0000 (15:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188520

Reviewed by Alex Christensen.

Source/WebKit:

willDestroySurface overwrites the surface pointer in the map's iterator in an attempt to
change the value of the surface pointer in the map, but it doesn't work because changing
the iterator does not change the map itself. There's no need to fix this function: it's
better to use WeakPtr instead.

* UIProcess/gtk/WaylandCompositor.cpp:
(WebKit::WaylandCompositor::getTexture):
(WebKit::WaylandCompositor::bindSurfaceToWebPage):
(WebKit::WaylandCompositor::unregisterWebPage):
(WebKit::WaylandCompositor::willDestroySurface): Deleted.
* UIProcess/gtk/WaylandCompositor.h:

Source/WTF:

Add a comment pointing to CanMakeWeakPtr, since it's useful and I very nearly missed it.

* wtf/WeakPtr.h:

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

Source/WTF/ChangeLog
Source/WTF/wtf/WeakPtr.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp
Source/WebKit/UIProcess/gtk/WaylandCompositor.h

index 37d41f2d37ebce7c0beccf687755a132511f4445..ba62c47ef94b76a8b12df8496bc69a5611c67276 100644 (file)
@@ -1,3 +1,14 @@
+2018-08-15  Michael Catanzaro  <mcatanzaro@igalia.com>
+
+        [WPE][GTK] WaylandCompositor fails to properly remove surface from its page map
+        https://bugs.webkit.org/show_bug.cgi?id=188520
+
+        Reviewed by Alex Christensen.
+
+        Add a comment pointing to CanMakeWeakPtr, since it's useful and I very nearly missed it.
+
+        * wtf/WeakPtr.h:
+
 2018-08-14  Saam barati  <sbarati@apple.com>
 
         HashMap<Ref<P>, V> asserts when V is not zero for its empty value
index eedd81f3f6c0b3efbd9be5d2b4884094be7eeda4..56f38664acbd8ae5314c244cf34deb7468ba39e9 100644 (file)
@@ -90,6 +90,7 @@ private:
     RefPtr<WeakReference<T>> m_ref;
 };
 
+// Note: you probably want to inherit from CanMakeWeakPtr rather than use this directly.
 template<typename T>
 class WeakPtrFactory {
     WTF_MAKE_NONCOPYABLE(WeakPtrFactory<T>);
index 8aff4173b9ee401f3209809d95483374775ba777..65b78a253c9de6d06d570c6bdfef158d67ac774b 100644 (file)
@@ -1,3 +1,22 @@
+2018-08-15  Michael Catanzaro  <mcatanzaro@igalia.com>
+
+        [WPE][GTK] WaylandCompositor fails to properly remove surface from its page map
+        https://bugs.webkit.org/show_bug.cgi?id=188520
+
+        Reviewed by Alex Christensen.
+
+        willDestroySurface overwrites the surface pointer in the map's iterator in an attempt to
+        change the value of the surface pointer in the map, but it doesn't work because changing
+        the iterator does not change the map itself. There's no need to fix this function: it's
+        better to use WeakPtr instead.
+
+        * UIProcess/gtk/WaylandCompositor.cpp:
+        (WebKit::WaylandCompositor::getTexture):
+        (WebKit::WaylandCompositor::bindSurfaceToWebPage):
+        (WebKit::WaylandCompositor::unregisterWebPage):
+        (WebKit::WaylandCompositor::willDestroySurface): Deleted.
+        * UIProcess/gtk/WaylandCompositor.h:
+
 2018-08-15  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [Attachment SPI] Remove attachment display mode options
index 8a8e6976d96080984e53e52413d5a3e56b9f3f7c..6da99830534f2295553f90c19d33a66e79fe2b29 100644 (file)
@@ -350,7 +350,6 @@ static const struct wl_compositor_interface compositorInterface = {
             wl_resource_set_implementation(surfaceResource, &surfaceInterface, new WaylandCompositor::Surface(),
                 [](struct wl_resource* resource) {
                     auto* surface = static_cast<WaylandCompositor::Surface*>(wl_resource_get_user_data(resource));
-                    WaylandCompositor::singleton().willDestroySurface(surface);
                     delete surface;
                 });
         } else
@@ -531,7 +530,7 @@ WaylandCompositor::WaylandCompositor()
 
 bool WaylandCompositor::getTexture(WebPageProxy& webPage, unsigned& texture, IntSize& textureSize)
 {
-    if (auto* surface = m_pageMap.get(&webPage))
+    if (WeakPtr<Surface> surface = m_pageMap.get(&webPage))
         return surface->prepareTextureForPainting(texture, textureSize);
     return false;
 }
@@ -549,7 +548,7 @@ void WaylandCompositor::bindSurfaceToWebPage(WaylandCompositor::Surface* surface
         return;
 
     surface->setWebPage(webPage);
-    m_pageMap.set(webPage, surface);
+    m_pageMap.set(webPage, makeWeakPtr(*surface));
 }
 
 void WaylandCompositor::registerWebPage(WebPageProxy& webPage)
@@ -559,20 +558,10 @@ void WaylandCompositor::registerWebPage(WebPageProxy& webPage)
 
 void WaylandCompositor::unregisterWebPage(WebPageProxy& webPage)
 {
-    if (auto* surface = m_pageMap.take(&webPage))
+    if (WeakPtr<Surface> surface = m_pageMap.take(&webPage))
         surface->setWebPage(nullptr);
 }
 
-void WaylandCompositor::willDestroySurface(Surface* surface)
-{
-    for (auto it : m_pageMap) {
-        if (it.value == surface) {
-            it.value = nullptr;
-            return;
-        }
-    }
-}
-
 } // namespace WebKit
 
 #endif // PLATFORM(WAYLAND) && USE(EGL)
index 6e5131193e535d081f38392d7f294b15f620c608..f6bf21d9e2ab26994e633b0b5846ebb247da3436 100644 (file)
@@ -76,7 +76,7 @@ public:
         uint32_t m_busyCount { 0 };
     };
 
-    class Surface {
+    class Surface : public CanMakeWeakPtr<Surface> {
         WTF_MAKE_NONCOPYABLE(Surface); WTF_MAKE_FAST_ALLOCATED;
     public:
         Surface();
@@ -111,7 +111,6 @@ public:
     void bindSurfaceToWebPage(Surface*, uint64_t pageID);
     void registerWebPage(WebPageProxy&);
     void unregisterWebPage(WebPageProxy&);
-    void willDestroySurface(Surface*);
 
     bool getTexture(WebPageProxy&, unsigned&, WebCore::IntSize&);
 
@@ -131,7 +130,7 @@ private:
     WebCore::WlUniquePtr<struct wl_global> m_webkitgtkGlobal;
     GRefPtr<GSource> m_eventSource;
     std::unique_ptr<WebCore::GLContext> m_eglContext;
-    HashMap<WebPageProxy*, Surface*> m_pageMap;
+    HashMap<WebPageProxy*, WeakPtr<Surface>> m_pageMap;
 };
 
 } // namespace WebKit