Changeset 249683 in webkit


Ignore:
Timestamp:
Sep 9, 2019 6:31:06 PM (5 years ago)
Author:
Fujii Hironori
Message:

[Win][MiniBrowser] WebKitLegacyBrowserWindow is leaked by circular references
​https://bugs.webkit.org/show_bug.cgi?id=201600

Reviewed by Brent Fulgham.

There were some circular references between
WebKitLegacyBrowserWindow and its delegation classes. For
example, WebKitLegacyBrowserWindow has a reference of
WebDownloadDelegate, and WebDownloadDelegate shares the ref
counter with WebKitLegacyBrowserWindow.

WebNotificationObserver was leaked because it wasn't unregistered
from the default notification center by using
IWebNotificationCenter::removeObserver.

If a new legacy window was created by mouse right click a link,
WebView was released twice because
PrintWebUIDelegate::createWebViewWithRequest didn't AddRef the
WebView.

This change does:

  1. Make delegation classes have own ref-counter to avoid circular references
  2. Do removeObserver notification observers
  3. AddRef WebView in PrintWebUIDelegate::createWebViewWithRequest
  • MiniBrowser/win/AccessibilityDelegate.cpp:

(AccessibilityDelegate::AddRef):
(AccessibilityDelegate::Release):

  • MiniBrowser/win/AccessibilityDelegate.h: Added m_refCount.
  • MiniBrowser/win/MiniBrowserWebHost.cpp:

(MiniBrowserWebHost::QueryInterface):
(MiniBrowserWebHost::AddRef):
(MiniBrowserWebHost::Release):

  • MiniBrowser/win/MiniBrowserWebHost.h: Added m_refCount.
  • MiniBrowser/win/PrintWebUIDelegate.cpp:

(PrintWebUIDelegate::createWebViewWithRequest): Do AddRef for the returned IWebView.
(PrintWebUIDelegate::AddRef):
(PrintWebUIDelegate::Release):

  • MiniBrowser/win/PrintWebUIDelegate.h: Added m_refCount.
  • MiniBrowser/win/ResourceLoadDelegate.cpp:

(ResourceLoadDelegate::AddRef):
(ResourceLoadDelegate::Release):

  • MiniBrowser/win/ResourceLoadDelegate.h: Added m_refCount.
  • MiniBrowser/win/WebDownloadDelegate.cpp:

(WebDownloadDelegate::AddRef):
(WebDownloadDelegate::Release):

  • MiniBrowser/win/WebDownloadDelegate.h: Added m_refCount.
  • MiniBrowser/win/WebKitLegacyBrowserWindow.cpp:

(WebKitLegacyBrowserWindow::~WebKitLegacyBrowserWindow): Do removeObserver notification observers.
(WebKitLegacyBrowserWindow::init):
(WebKitLegacyBrowserWindow::setUIDelegate):
(WebKitLegacyBrowserWindow::setAccessibilityDelegate):
(WebKitLegacyBrowserWindow::setResourceLoadDelegate):
(WebKitLegacyBrowserWindow::setDownloadDelegate):
(WebKitLegacyBrowserWindow::AddRef): Deleted.
(WebKitLegacyBrowserWindow::Release): Deleted.
(WebKitLegacyBrowserWindow::setFrameLoadDelegate): Deleted.
(WebKitLegacyBrowserWindow::setFrameLoadDelegatePrivate): Deleted.

  • MiniBrowser/win/WebKitLegacyBrowserWindow.h:
Location:
trunk/Tools
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r249675 r249683  
     12019-09-09  Fujii Hironori  <Hironori.Fujii@sony.com>
     2
     3        [Win][MiniBrowser] WebKitLegacyBrowserWindow is leaked by circular references
     4        https://bugs.webkit.org/show_bug.cgi?id=201600
     5
     6        Reviewed by Brent Fulgham.
     7
     8        There were some circular references between
     9        WebKitLegacyBrowserWindow and its delegation classes. For
     10        example, WebKitLegacyBrowserWindow has a reference of
     11        WebDownloadDelegate, and WebDownloadDelegate shares the ref
     12        counter with WebKitLegacyBrowserWindow.
     13
     14        WebNotificationObserver was leaked because it wasn't unregistered
     15        from the default notification center by using
     16        IWebNotificationCenter::removeObserver.
     17
     18        If a new legacy window was created by mouse right click a link,
     19        WebView was released twice because
     20        PrintWebUIDelegate::createWebViewWithRequest didn't AddRef the
     21        WebView.
     22
     23        This change does:
     24        1. Make delegation classes have own ref-counter to avoid circular references
     25        2. Do removeObserver notification observers
     26        3. AddRef WebView in PrintWebUIDelegate::createWebViewWithRequest
     27
     28        * MiniBrowser/win/AccessibilityDelegate.cpp:
     29        (AccessibilityDelegate::AddRef):
     30        (AccessibilityDelegate::Release):
     31        * MiniBrowser/win/AccessibilityDelegate.h: Added m_refCount.
     32        * MiniBrowser/win/MiniBrowserWebHost.cpp:
     33        (MiniBrowserWebHost::QueryInterface):
     34        (MiniBrowserWebHost::AddRef):
     35        (MiniBrowserWebHost::Release):
     36        * MiniBrowser/win/MiniBrowserWebHost.h: Added m_refCount.
     37        * MiniBrowser/win/PrintWebUIDelegate.cpp:
     38        (PrintWebUIDelegate::createWebViewWithRequest): Do AddRef for the returned IWebView.
     39        (PrintWebUIDelegate::AddRef):
     40        (PrintWebUIDelegate::Release):
     41        * MiniBrowser/win/PrintWebUIDelegate.h: Added m_refCount.
     42        * MiniBrowser/win/ResourceLoadDelegate.cpp:
     43        (ResourceLoadDelegate::AddRef):
     44        (ResourceLoadDelegate::Release):
     45        * MiniBrowser/win/ResourceLoadDelegate.h: Added m_refCount.
     46        * MiniBrowser/win/WebDownloadDelegate.cpp:
     47        (WebDownloadDelegate::AddRef):
     48        (WebDownloadDelegate::Release):
     49        * MiniBrowser/win/WebDownloadDelegate.h: Added m_refCount.
     50        * MiniBrowser/win/WebKitLegacyBrowserWindow.cpp:
     51        (WebKitLegacyBrowserWindow::~WebKitLegacyBrowserWindow): Do removeObserver notification observers.
     52        (WebKitLegacyBrowserWindow::init):
     53        (WebKitLegacyBrowserWindow::setUIDelegate):
     54        (WebKitLegacyBrowserWindow::setAccessibilityDelegate):
     55        (WebKitLegacyBrowserWindow::setResourceLoadDelegate):
     56        (WebKitLegacyBrowserWindow::setDownloadDelegate):
     57        (WebKitLegacyBrowserWindow::AddRef): Deleted.
     58        (WebKitLegacyBrowserWindow::Release): Deleted.
     59        (WebKitLegacyBrowserWindow::setFrameLoadDelegate): Deleted.
     60        (WebKitLegacyBrowserWindow::setFrameLoadDelegatePrivate): Deleted.
     61        * MiniBrowser/win/WebKitLegacyBrowserWindow.h:
     62
    1632019-09-09  Chris Dumez  <cdumez@apple.com>
    264

  • trunk/Tools/MiniBrowser/win/AccessibilityDelegate.cpp

    r232669 r249683  
    5454ULONG AccessibilityDelegate::AddRef()
    5555{
    56     return m_client.AddRef();
     56    return ++m_refCount;
    5757}
    5858
    5959ULONG AccessibilityDelegate::Release()
    6060{
    61     return m_client.Release();
     61    ULONG newRef = --m_refCount;
     62    if (!newRef)
     63        delete this;
     64    return newRef;
    6265}
    6366

  • trunk/Tools/MiniBrowser/win/AccessibilityDelegate.h

    r232669 r249683  
    4242    virtual HRESULT STDMETHODCALLTYPE fireFrameLoadFinishedEvents();
    4343private:
     44    ULONG m_refCount { 0 };
    4445    WebKitLegacyBrowserWindow& m_client;
    4546};

  • trunk/Tools/MiniBrowser/win/MiniBrowserWebHost.cpp

    r249039 r249683  
    8383    else if (IsEqualGUID(riid, IID_IWebFrameLoadDelegate))
    8484        *ppvObject = static_cast<IWebFrameLoadDelegate*>(this);
     85    else if (IsEqualGUID(riid, IID_IWebFrameLoadDelegatePrivate))
     86        *ppvObject = static_cast<IWebFrameLoadDelegatePrivate*>(this);
     87    else if (IsEqualGUID(riid, IID_IWebNotificationObserver))
     88        *ppvObject = static_cast<IWebNotificationObserver*>(this);
    8589    else
    8690        return E_NOINTERFACE;
    … …  
    9296ULONG MiniBrowserWebHost::AddRef()
    9397{
    94     return m_client->AddRef();
     98    return ++m_refCount;
    9599}
    96100
    97101ULONG MiniBrowserWebHost::Release()
    98102{
    99     return m_client->Release();
     103    ULONG newRef = --m_refCount;
     104    if (!newRef)
     105        delete this;
     106    return newRef;
    100107}
    101108

  • trunk/Tools/MiniBrowser/win/MiniBrowserWebHost.h

    r249039 r249683  
    6969
    7070private:
     71    ULONG m_refCount { 0 };
    7172    WebKitLegacyBrowserWindow* m_client { nullptr };
    7273};

  • trunk/Tools/MiniBrowser/win/PrintWebUIDelegate.cpp

    r249039 r249683  
    7373
    7474    auto& newBrowserWindow = *static_cast<WebKitLegacyBrowserWindow*>(newWindow.browserWindow());
    75     *newWebView = newBrowserWindow.webView();
     75    *newWebView = newBrowserWindow.webView().Detach();
    7676    IWebFramePtr frame;
    7777    HRESULT hr;
    … …  
    152152ULONG PrintWebUIDelegate::AddRef()
    153153{
    154     return m_client.AddRef();
     154    return ++m_refCount;
    155155}
    156156
    157157ULONG PrintWebUIDelegate::Release()
    158158{
    159     return m_client.Release();
     159    ULONG newRef = --m_refCount;
     160    if (!newRef)
     161        delete this;
     162    return newRef;
    160163}
    161164

  • trunk/Tools/MiniBrowser/win/PrintWebUIDelegate.h

    r232669 r249683  
    107107
    108108private:
     109    ULONG m_refCount { 0 };
    109110    WebKitLegacyBrowserWindow& m_client;
    110111    HWND m_modalDialogParent { nullptr };

  • trunk/Tools/MiniBrowser/win/ResourceLoadDelegate.cpp

    r247896 r249683  
    5656ULONG ResourceLoadDelegate::AddRef()
    5757{
    58     return m_client->AddRef();
     58    return ++m_refCount;
    5959}
    6060
    6161ULONG ResourceLoadDelegate::Release()
    6262{
    63     return m_client->Release();
     63    ULONG newRef = --m_refCount;
     64    if (!newRef)
     65        delete this;
     66    return newRef;
    6467}
    6568

  • trunk/Tools/MiniBrowser/win/ResourceLoadDelegate.h

    r232669 r249683  
    5353
    5454private:
     55    ULONG m_refCount { 0 };
    5556    WebKitLegacyBrowserWindow* m_client;
    5657};

  • trunk/Tools/MiniBrowser/win/WebDownloadDelegate.cpp

    r232669 r249683  
    6060ULONG WebDownloadDelegate::AddRef()
    6161{
    62     return m_client.AddRef();
     62    return ++m_refCount;
    6363}
    6464
    6565ULONG WebDownloadDelegate::Release()
    6666{
    67     return m_client.Release();
     67    ULONG newRef = --m_refCount;
     68    if (!newRef)
     69        delete this;
     70    return newRef;
    6871}
    6972

  • trunk/Tools/MiniBrowser/win/WebDownloadDelegate.h

    r232669 r249683  
    5555
    5656private:
     57    ULONG m_refCount { 0 };
    5758    WebKitLegacyBrowserWindow& m_client;
    5859};

  • trunk/Tools/MiniBrowser/win/WebKitLegacyBrowserWindow.cpp

    r249039 r249683  
    7070}
    7171
    72 ULONG WebKitLegacyBrowserWindow::AddRef()
    73 {
    74     ref();
    75     return refCount();
    76 }
    77 
    78 ULONG WebKitLegacyBrowserWindow::Release()
    79 {
    80     auto count = refCount();
    81     deref();
    82     return --count;
     72WebKitLegacyBrowserWindow::~WebKitLegacyBrowserWindow()
     73{
     74    m_defaultNotificationCenter->removeObserver(m_notificationObserver, _bstr_t(WebViewProgressEstimateChangedNotification), m_webView);
     75    m_defaultNotificationCenter->removeObserver(m_notificationObserver, _bstr_t(WebViewProgressFinishedNotification), m_webView);
    8376}
    8477
    … …  
    110103        return hr;
    111104
    112     IWebNotificationCenterPtr defaultNotificationCenter;
    113     hr = notificationCenter->defaultCenter(&defaultNotificationCenter.GetInterfacePtr());
     105    hr = notificationCenter->defaultCenter(&m_defaultNotificationCenter.GetInterfacePtr());
    114106    if (FAILED(hr))
    115107        return hr;
    … …  
    126118    auto webHost = new MiniBrowserWebHost(this);
    127119
    128     hr = setFrameLoadDelegate(webHost);
    129     if (FAILED(hr))
    130         return hr;
    131 
    132     hr = setFrameLoadDelegatePrivate(webHost);
    133     if (FAILED(hr))
    134         return hr;
    135 
    136     hr = defaultNotificationCenter->addObserver(webHost, _bstr_t(WebViewProgressEstimateChangedNotification), nullptr);
    137     if (FAILED(hr))
    138         return hr;
    139 
    140     hr = defaultNotificationCenter->addObserver(webHost, _bstr_t(WebViewProgressFinishedNotification), nullptr);
     120    hr = m_webView->setFrameLoadDelegate(webHost);
     121    if (FAILED(hr))
     122        return hr;
     123
     124    hr = m_webViewPrivate->setFrameLoadDelegatePrivate(webHost);
     125    if (FAILED(hr))
     126        return hr;
     127
     128    m_notificationObserver = webHost;
     129    hr = m_defaultNotificationCenter->addObserver(m_notificationObserver, _bstr_t(WebViewProgressEstimateChangedNotification), m_webView);
     130    if (FAILED(hr))
     131        return hr;
     132
     133    hr = m_defaultNotificationCenter->addObserver(m_notificationObserver, _bstr_t(WebViewProgressFinishedNotification), m_webView);
    141134    if (FAILED(hr))
    142135        return hr;
    … …  
    154147        return hr;
    155148
    156     IWebDownloadDelegatePtr downloadDelegate;
    157     downloadDelegate.Attach(new WebDownloadDelegate(*this));
    158     hr = setDownloadDelegate(downloadDelegate);
     149    hr = setDownloadDelegate(new WebDownloadDelegate(*this));
    159150    if (FAILED(hr))
    160151        return hr;
    … …  
    248239}
    249240
    250 HRESULT WebKitLegacyBrowserWindow::setFrameLoadDelegate(IWebFrameLoadDelegate* frameLoadDelegate)
    251 {
    252     m_frameLoadDelegate = frameLoadDelegate;
    253     return m_webView->setFrameLoadDelegate(frameLoadDelegate);
    254 }
    255 
    256 HRESULT WebKitLegacyBrowserWindow::setFrameLoadDelegatePrivate(IWebFrameLoadDelegatePrivate* frameLoadDelegatePrivate)
    257 {
    258     return m_webViewPrivate->setFrameLoadDelegatePrivate(frameLoadDelegatePrivate);
    259 }
    260 
    261241HRESULT WebKitLegacyBrowserWindow::setUIDelegate(IWebUIDelegate* uiDelegate)
    262242{
    263     m_uiDelegate = uiDelegate;
    264243    return m_webView->setUIDelegate(uiDelegate);
    265244}
    … …  
    267246HRESULT WebKitLegacyBrowserWindow::setAccessibilityDelegate(IAccessibilityDelegate* accessibilityDelegate)
    268247{
    269     m_accessibilityDelegate = accessibilityDelegate;
    270248    return m_webView->setAccessibilityDelegate(accessibilityDelegate);
    271249}
    … …  
    273251HRESULT WebKitLegacyBrowserWindow::setResourceLoadDelegate(IWebResourceLoadDelegate* resourceLoadDelegate)
    274252{
    275     m_resourceLoadDelegate = resourceLoadDelegate;
    276253    return m_webView->setResourceLoadDelegate(resourceLoadDelegate);
    277254}
    … …  
    279256HRESULT WebKitLegacyBrowserWindow::setDownloadDelegate(IWebDownloadDelegatePtr downloadDelegate)
    280257{
    281     m_downloadDelegate = downloadDelegate;
    282258    return m_webView->setDownloadDelegate(downloadDelegate);
    283259}

  • trunk/Tools/MiniBrowser/win/WebKitLegacyBrowserWindow.h

    r249039 r249683  
    6565    friend class ResourceLoadDelegate;
    6666
    67     ULONG AddRef();
    68     ULONG Release();
    69 
    7067    HRESULT init();
    7168    HRESULT prepareViews(HWND mainWnd, const RECT& clientRect);
    … …  
    8279    bool setToDefaultPreferences();
    8380
    84     HRESULT setFrameLoadDelegate(IWebFrameLoadDelegate*);
    85     HRESULT setFrameLoadDelegatePrivate(IWebFrameLoadDelegatePrivate*);
    8681    HRESULT setUIDelegate(IWebUIDelegate*);
    8782    HRESULT setAccessibilityDelegate(IAccessibilityDelegate*);
    … …  
    117112
    118113    WebKitLegacyBrowserWindow(BrowserWindowClient&, HWND mainWnd, bool useLayeredWebView);
     114    ~WebKitLegacyBrowserWindow();
    119115    void subclassForLayeredWindow();
    120116    bool setCacheFolder();
    … …  
    130126    IWebPreferencesPtr m_standardPreferences;
    131127    IWebPreferencesPrivatePtr m_prefsPrivate;
    132 
    133     IWebFrameLoadDelegatePtr m_frameLoadDelegate;
    134     IWebUIDelegatePtr m_uiDelegate;
    135     IAccessibilityDelegatePtr m_accessibilityDelegate;
    136     IWebResourceLoadDelegatePtr m_resourceLoadDelegate;
    137     IWebDownloadDelegatePtr m_downloadDelegate;
     128    IWebNotificationCenterPtr m_defaultNotificationCenter;
     129    IWebNotificationObserverPtr m_notificationObserver;
    138130
    139131    IWebCoreStatisticsPtr m_statistics;
Note: See TracChangeset for help on using the changeset viewer.