Bug 186263 - [Win][MiniBrowser] Support multiple windows properly
Summary: [Win][MiniBrowser] Support multiple windows properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks: 184770
  Show dependency treegraph
 
Reported: 2018-06-04 01:10 PDT by Fujii Hironori
Modified: 2019-09-08 23:32 PDT (History)
8 users (show)

See Also:


Attachments
Patch (18.15 KB, patch)
2018-06-04 01:53 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (18.16 KB, patch)
2018-06-04 02:00 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.77 MB, application/zip)
2018-06-04 09:03 PDT, Build Bot
no flags Details
Patch (23.03 KB, patch)
2018-06-05 21:59 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (23.17 KB, patch)
2018-06-05 22:19 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (27.65 KB, patch)
2018-06-07 00:21 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2018-06-04 01:10:51 PDT
[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
Comment 1 Fujii Hironori 2018-06-04 01:53:48 PDT
Created attachment 341896 [details]
Patch
Comment 2 Fujii Hironori 2018-06-04 02:00:36 PDT
Created attachment 341897 [details]
Patch
Comment 3 Build Bot 2018-06-04 09:03:05 PDT
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
Comment 4 Build Bot 2018-06-04 09:03:16 PDT
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
Comment 5 Fujii Hironori 2018-06-04 14:04:11 PDT
Anyone have a time to review my patch?
Comment 6 Fujii Hironori 2018-06-05 13:50:05 PDT
Could you please review?
Comment 7 Ryosuke Niwa 2018-06-05 17:31:09 PDT
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.
Comment 8 Fujii Hironori 2018-06-05 18:37:09 PDT
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.
Comment 9 Ryosuke Niwa 2018-06-05 18:57:28 PDT
(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.
Comment 10 Ryosuke Niwa 2018-06-05 18:57:44 PDT
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.
Comment 11 Fujii Hironori 2018-06-05 21:59:11 PDT
Created attachment 342031 [details]
Patch
Comment 12 Fujii Hironori 2018-06-05 22:04:48 PDT
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.
Comment 13 Fujii Hironori 2018-06-05 22:19:12 PDT
Created attachment 342034 [details]
Patch
Comment 14 Fujii Hironori 2018-06-06 03:34:10 PDT
(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).
Comment 15 Ryosuke Niwa 2018-06-06 22:53:45 PDT
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.
Comment 16 Fujii Hironori 2018-06-06 23:35:58 PDT
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.
Comment 17 Fujii Hironori 2018-06-07 00:21:57 PDT
Created attachment 342132 [details]
Patch
Comment 18 Fujii Hironori 2018-06-07 00:32:22 PDT
Committed r232577: <https://trac.webkit.org/changeset/232577>
Comment 19 Radar WebKit Bug Importer 2018-06-07 00:39:05 PDT
<rdar://problem/40884426>
Comment 20 Fujii Hironori 2019-09-08 23:32:30 PDT
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.