[webkit-reviews] review denied: [Bug 186263] [Win][MiniBrowser] Support multiple windows properly : [Attachment 341897] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 186263: [Win][MiniBrowser] Support multiple windows properly
https://bugs.webkit.org/show_bug.cgi?id=186263

Attachment 341897: Patch

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




--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 341897
  --> https://bugs.webkit.org/attachment.cgi?id=341897
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.cp
p?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.