Bug 186263 – [Win][MiniBrowser] Support multiple windows properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fujii Hironori]

[Win][MiniBrowser] Support multiple windows properly

The current implementation of
PrintWebUIDelegate::createWebViewWithRequest is wrong. It is
using CreateProcess to open a new window, and doesn't return the
new instance of IWebView. As the result, for example,
window.close doesn't work as expected.

A test case of window.close
http://javascript-coder.com/files/window-popup/javascript-window-close-example1.html

[Fujii Hironori]

Created attachment 341896 [details]
Patch

[Fujii Hironori]

Created attachment 341897 [details]
Patch

[EWS Watchlist]

Comment on attachment 341897 [details]
Patch

Attachment 341897 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7978194

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 341905 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Fujii Hironori]

Anyone have a time to review my patch?

[Fujii Hironori]

Could you please review?

[Ryosuke Niwa]

Comment on attachment 341897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341897&action=review

> Tools/MiniBrowser/win/MainWindow.cpp:112
> +    m_browserWindow = new MiniBrowser(m_hMainWnd, m_hURLBarWnd, usesLayeredWebView, pageLoadTesting);

Tying the life cycle of MiniBrowser to delegate is one thing but this makes it very unclear who owns MiniBrowser.
I think MainWindow should explicitly ref MiniBrowser, and clear it in WM_DESTROY.

> Tools/MiniBrowser/win/MainWindow.cpp:218
> +        delete &thiz;
> +        if (s_numInstances)
> +            return 0;

It's almost never safe to "delete this" / the object stored in GetWindowLongPtr due to re-entrancy.
For example, PrintWebUIDelegate::webViewClose accesses m_modalDialogParent after invoking DestroyWindow.
Deleting MainWindow would never destroy PrintWebUIDelegate?

A better approach is to make MainWindow RefCounted, and do ref() / deref() pair in WM_CREATE & WM_DESTROY,
and then ref() / deref() whenever we enter this function.

[Fujii Hironori]

Comment on attachment 341897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341897&action=review

Thank you very much for the review, Ryosuke.

>> Tools/MiniBrowser/win/MainWindow.cpp:112
>> +    m_browserWindow = new MiniBrowser(m_hMainWnd, m_hURLBarWnd, usesLayeredWebView, pageLoadTesting);
> 
> Tying the life cycle of MiniBrowser to delegate is one thing but this makes it very unclear who owns MiniBrowser.
> I think MainWindow should explicitly ref MiniBrowser, and clear it in WM_DESTROY.

MiniBrowser is a ref-counted object. It is owned by all its
ref-count holders. It's a common practice that newly created
instance has just one ref count in COM and even in WebKit. It's a
weird practice to ref explicitly a newly created instance.

MainBrowser owns one ref count of MiniBrowser. I think the
destructor of MainBrowser should deref the instance of
MiniBrowser.

>> Tools/MiniBrowser/win/MainWindow.cpp:218
>> +            return 0;
> 
> It's almost never safe to "delete this" / the object stored in GetWindowLongPtr due to re-entrancy.
> For example, PrintWebUIDelegate::webViewClose accesses m_modalDialogParent after invoking DestroyWindow.
> Deleting MainWindow would never destroy PrintWebUIDelegate?
> 
> A better approach is to make MainWindow RefCounted, and do ref() / deref() pair in WM_CREATE & WM_DESTROY,
> and then ref() / deref() whenever we enter this function.

I agree it can be possible to post WM_DESTROY during WM_DESTROY
is processed. But, I don't think I need to care the case. If such
thing happens, a real disaster will come, for example the window
and child windows would be destroyed twice.

webViewClose() is called in a following code.
https://trac.webkit.org/browser/webkit/trunk/Source/WebKitLegacy/win/WebView.cpp?rev=232499#L1544
COMPtr<IWebUIDelegate> holds a ref count of PrintWebUIDelegate.
And. the substance of the ref counter of PrintWebUIDelegate is one of
MiniBrowser. MiniBrowser will live longer than MainWindow.

[Ryosuke Niwa]

(In reply to Fujii Hironori from comment #8)
> Comment on attachment 341897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341897&action=review
> 
> Thank you very much for the review, Ryosuke.
> 
> >> Tools/MiniBrowser/win/MainWindow.cpp:112
> >> +    m_browserWindow = new MiniBrowser(m_hMainWnd, m_hURLBarWnd, usesLayeredWebView, pageLoadTesting);
> > 
> > Tying the life cycle of MiniBrowser to delegate is one thing but this makes it very unclear who owns MiniBrowser.
> > I think MainWindow should explicitly ref MiniBrowser, and clear it in WM_DESTROY.
> 
> MiniBrowser is a ref-counted object. It is owned by all its
> ref-count holders. It's a common practice that newly created
> instance has just one ref count in COM and even in WebKit. It's a
> weird practice to ref explicitly a newly created instance.
> 
> MainBrowser owns one ref count of MiniBrowser. I think the
> destructor of MainBrowser should deref the instance of
> MiniBrowser.

What I'm suggesting is to use COMPtr or RefPtr to do that automatically.

> >> Tools/MiniBrowser/win/MainWindow.cpp:218
> >> +            return 0;
> > 
> > It's almost never safe to "delete this" / the object stored in GetWindowLongPtr due to re-entrancy.
> > For example, PrintWebUIDelegate::webViewClose accesses m_modalDialogParent after invoking DestroyWindow.
> > Deleting MainWindow would never destroy PrintWebUIDelegate?
> > 
> > A better approach is to make MainWindow RefCounted, and do ref() / deref() pair in WM_CREATE & WM_DESTROY,
> > and then ref() / deref() whenever we enter this function.
> 
> I agree it can be possible to post WM_DESTROY during WM_DESTROY
> is processed. But, I don't think I need to care the case. If such
> thing happens, a real disaster will come, for example the window
> and child windows would be destroyed twice.

Well, that's not the only case. It's totally possible to invoke DestroyWindow in the midst of processing another message to the window (e.g. WM_CLOSE or WM_COMMAND).

> webViewClose() is called in a following code.
> https://trac.webkit.org/browser/webkit/trunk/Source/WebKitLegacy/win/WebView.
> cpp?rev=232499#L1544
> COMPtr<IWebUIDelegate> holds a ref count of PrintWebUIDelegate.
> And. the substance of the ref counter of PrintWebUIDelegate is one of
> MiniBrowser. MiniBrowser will live longer than MainWindow.

I don't think we want to be doing that kind of analysis every time new code is added to MiniBrowser to make sure we don't use use-after-free.

[Ryosuke Niwa]

Comment on attachment 341897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341897&action=review

>>>> Tools/MiniBrowser/win/MainWindow.cpp:218
>>>> +            return 0;
>>> 
>>> It's almost never safe to "delete this" / the object stored in GetWindowLongPtr due to re-entrancy.
>>> For example, PrintWebUIDelegate::webViewClose accesses m_modalDialogParent after invoking DestroyWindow.
>>> Deleting MainWindow would never destroy PrintWebUIDelegate?
>>> 
>>> A better approach is to make MainWindow RefCounted, and do ref() / deref() pair in WM_CREATE & WM_DESTROY,
>>> and then ref() / deref() whenever we enter this function.
>> 
>> I agree it can be possible to post WM_DESTROY during WM_DESTROY
>> is processed. But, I don't think I need to care the case. If such
>> thing happens, a real disaster will come, for example the window
>> and child windows would be destroyed twice.
>> 
>> webViewClose() is called in a following code.
>> https://trac.webkit.org/browser/webkit/trunk/Source/WebKitLegacy/win/WebView.cpp?rev=232499#L1544
>> COMPtr<IWebUIDelegate> holds a ref count of PrintWebUIDelegate.
>> And. the substance of the ref counter of PrintWebUIDelegate is one of
>> MiniBrowser. MiniBrowser will live longer than MainWindow.
> 
> Well, that's not the only case. It's totally possible to invoke DestroyWindow in the midst of processing another message to the window (e.g. WM_CLOSE or WM_COMMAND).

r- because of this issue.

[Fujii Hironori]

Created attachment 342031 [details]
Patch

[Fujii Hironori]

Comment on attachment 341897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341897&action=review

>>>> Tools/MiniBrowser/win/MainWindow.cpp:112
>>>> +    m_browserWindow = new MiniBrowser(m_hMainWnd, m_hURLBarWnd, usesLayeredWebView, pageLoadTesting);
>>> 
>>> Tying the life cycle of MiniBrowser to delegate is one thing but this makes it very unclear who owns MiniBrowser.
>>> I think MainWindow should explicitly ref MiniBrowser, and clear it in WM_DESTROY.
>> 
>> MiniBrowser is a ref-counted object. It is owned by all its
>> ref-count holders. It's a common practice that newly created
>> instance has just one ref count in COM and even in WebKit. It's a
>> weird practice to ref explicitly a newly created instance.
>> 
>> MainBrowser owns one ref count of MiniBrowser. I think the
>> destructor of MainBrowser should deref the instance of
>> MiniBrowser.
> 
> What I'm suggesting is to use COMPtr or RefPtr to do that automatically.

I will use RefPtr for them. If I use COM, I need to implement
unnecessary QueryInterface method and BrowserWindow (Will be
introduced in Bug 18477) needs IDL.

>>>>> Tools/MiniBrowser/win/MainWindow.cpp:218
>>>>> +            return 0;
>>>> 
>>>> It's almost never safe to "delete this" / the object stored in GetWindowLongPtr due to re-entrancy.
>>>> For example, PrintWebUIDelegate::webViewClose accesses m_modalDialogParent after invoking DestroyWindow.
>>>> Deleting MainWindow would never destroy PrintWebUIDelegate?
>>>> 
>>>> A better approach is to make MainWindow RefCounted, and do ref() / deref() pair in WM_CREATE & WM_DESTROY,
>>>> and then ref() / deref() whenever we enter this function.
>>> 
>>> I agree it can be possible to post WM_DESTROY during WM_DESTROY
>>> is processed. But, I don't think I need to care the case. If such
>>> thing happens, a real disaster will come, for example the window
>>> and child windows would be destroyed twice.
>>> 
>>> webViewClose() is called in a following code.
>>> https://trac.webkit.org/browser/webkit/trunk/Source/WebKitLegacy/win/WebView.cpp?rev=232499#L1544
>>> COMPtr<IWebUIDelegate> holds a ref count of PrintWebUIDelegate.
>>> And. the substance of the ref counter of PrintWebUIDelegate is one of
>>> MiniBrowser. MiniBrowser will live longer than MainWindow.
>> 
>> Well, that's not the only case. It's totally possible to invoke DestroyWindow in the midst of processing another message to the window (e.g. WM_CLOSE or WM_COMMAND).
> 
> r- because of this issue.

After calling DestroyWindow, you can't use the HWND.
I don't think MainWindow should be kept alive.
But, I'm not confident. I will make MainWindow ref-counted.

[Fujii Hironori]

Created attachment 342034 [details]
Patch

[Fujii Hironori]

(In reply to Ryosuke Niwa from comment #9)
> I don't think we want to be doing that kind of analysis every time new code
> is added to MiniBrowser to make sure we don't use use-after-free.

This change unifies the lifetime of MiniBrowser and its delegates.
The delegates doesn't live longer than MiniBrowser anymore.
We don't need to worry about the lifetime of them anymore.

One exception is that the child browser window (MiniBroser) lives longer than the parent window (MainWindow).

[Ryosuke Niwa]

Comment on attachment 342034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342034&action=review

> Tools/MiniBrowser/win/MainWindow.cpp:110
> -    m_browserWindow = std::make_unique<MiniBrowser>(m_hMainWnd, m_hURLBarWnd, usesLayeredWebView, pageLoadTesting);
> +    m_browserWindow = adoptRef(new MiniBrowser(m_hMainWnd, m_hURLBarWnd, usesLayeredWebView, pageLoadTesting));

Please add MiniBrowser::create as we do elsewhere in WebKit instead.

> Tools/MiniBrowser/win/MainWindow.cpp:146
> +    RefPtr<MainWindow> thiz = reinterpret_cast<MainWindow*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));

Can we just call this thisWindow instead of thiz?

> Tools/MiniBrowser/win/MiniBrowser.cpp:79
> +    auto c = refCount();

Please don't use a single letter variable like c. Call it count instead.

[Fujii Hironori]

Comment on attachment 342034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342034&action=review

Thank you very much for r+.

>> Tools/MiniBrowser/win/MainWindow.cpp:110
>> +    m_browserWindow = adoptRef(new MiniBrowser(m_hMainWnd, m_hURLBarWnd, usesLayeredWebView, pageLoadTesting));
> 
> Please add MiniBrowser::create as we do elsewhere in WebKit instead.

Will fix.

>> Tools/MiniBrowser/win/MainWindow.cpp:146
>> +    RefPtr<MainWindow> thiz = reinterpret_cast<MainWindow*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));
> 
> Can we just call this thisWindow instead of thiz?

Sounds good. Fix will.

>> Tools/MiniBrowser/win/MiniBrowser.cpp:79
>> +    auto c = refCount();
> 
> Please don't use a single letter variable like c. Call it count instead.

Will fix.

[Fujii Hironori]

Created attachment 342132 [details]
Patch

[Fujii Hironori]

Committed r232577: <https://trac.webkit.org/changeset/232577>

[Radar WebKit Bug Importer]

<rdar://problem/40884426>

[Fujii Hironori]

Comment on attachment 342132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342132&action=review

> Tools/MiniBrowser/win/PrintWebUIDelegate.cpp:72
> +    *newWebView = newWindow.browserWindow()->webView();

The returned IWebView should be AddRef-ed. I'm going to fix it in Bug 201600.