[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
Created attachment 341896 [details] Patch
Created attachment 341897 [details] Patch
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
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
Anyone have a time to review my patch?
Could you please review?
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 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.
(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 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.
Created attachment 342031 [details] Patch
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.
Created attachment 342034 [details] Patch
(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 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 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.
Created attachment 342132 [details] Patch
Committed r232577: <https://trac.webkit.org/changeset/232577>
<rdar://problem/40884426>
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.