Bug 103947 – [Chromium] Detach the frame and destroy the page immediately upon request to closeHelperPlugin().

• Component: New Bugs
• Status: RESOLVED FIXED

[David Dorwin]

[Chromium] Detach the frame and destroy the page immediately upon request to closeHelperPlugin().

[David Dorwin]

Created attachment 177370 [details]
Patch

[David Dorwin]

Regarding the WebPagePopupImpl changes, I'm not sure if m_page might be needed between closePopup() and close(). animate(), layout(), and paint() are the only calls that don't have a check for a null m_page. Destroying the page in a function that is the opposite of the one that creates it seems cleaner.

I'm not sure if WebWidget::close() would be called without closeHelperPlugin()/closePopup() so I've left a second call to dinitPage() in WebPagePopupImpl::close().

[David Dorwin]

tkent, please take a look since this this affects WebPagePopupImpl and WebHelperPluginImpl is based on WebPagePopupImpl. Thanks.

[Kent Tamura]

Comment on attachment 177370 [details]
Patch

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

For WebPagePopupImpl, I think m_page was necessary until close() because it received events. Now it has m_closing flag and we may destroy m_page in closePopup.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:142
> +    deinitPage();

nit: The word 'deinit' is not popular in WebKit.  We had better rename it to destroyPage, destructPage, or closePage?

Note that we should have given full-spelled names to 'init' and 'initPage'.

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:217
> +    // Initiate shutdown. This will cause a lot of notifications to be sent.

I guess this comment explained a reason of RefPtr<WebFrameImpl>, and we don't need the comment in this case.

> Source/WebKit/chromium/src/WebViewImpl.cpp:-1536
> -    RefPtr<WebFrameImpl> mainFrameImpl;
> -
>      if (m_page) {
>          // Initiate shutdown for the entire frameset.  This will cause a lot of
>          // notifications to be sent.
>          if (m_page->mainFrame()) {
> -            mainFrameImpl = WebFrameImpl::fromFrame(m_page->mainFrame());

I'm not sure if this is really unnecessary.

[David Dorwin]

Comment on attachment 177370 [details]
Patch

Thanks!

> For WebPagePopupImpl, I think m_page was necessary until close() because it received events. Now it has m_closing flag and we may destroy m_page in closePopup.

Are you saying that m_closing should protect us or that destroying m_page in closePopup() is a problem? Should I add checks to animate(), layout(), and paint()?


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

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:142
>> +    deinitPage();
> 
> nit: The word 'deinit' is not popular in WebKit.  We had better rename it to destroyPage, destructPage, or closePage?
> 
> Note that we should have given full-spelled names to 'init' and 'initPage'.

Okay, I will change it. Do you have a preference for destroy vs. close? Shall I also change init to initialize/create or is that too far outside the scope of this CL? (The naming may depend on whether we actually destroy m_page.)

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:217
>> +    // Initiate shutdown. This will cause a lot of notifications to be sent.
> 
> I guess this comment explained a reason of RefPtr<WebFrameImpl>, and we don't need the comment in this case.

I'm not sure it was related to that (see my comment in WebViewImpl.cpp below). I'll delete it anyway.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:-1536
>> -            mainFrameImpl = WebFrameImpl::fromFrame(m_page->mainFrame());
> 
> I'm not sure if this is really unnecessary.

I was unsure as well until I looked at where it was added [1]. If present mainFrameImpl, was used again at line 779. Thus, I think it was just to provide access (and ensure it was not destroyed until that access). I don't think this is responsible for managing the lifetime anymore.

[1] http://trac.webkit.org/browser/trunk/WebKit/chromium/src/WebViewImpl.cpp?rev=50739#L761

[Kent Tamura]

Comment on attachment 177370 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:142
>>> +    deinitPage();
>> 
>> nit: The word 'deinit' is not popular in WebKit.  We had better rename it to destroyPage, destructPage, or closePage?
>> 
>> Note that we should have given full-spelled names to 'init' and 'initPage'.
> 
> Okay, I will change it. Do you have a preference for destroy vs. close? Shall I also change init to initialize/create or is that too far outside the scope of this CL? (The naming may depend on whether we actually destroy m_page.)

I prefer destoryPage.

I don't think you should rename init/initPage in this bug.

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:-1536
>>> -            mainFrameImpl = WebFrameImpl::fromFrame(m_page->mainFrame());
>> 
>> I'm not sure if this is really unnecessary.
> 
> I was unsure as well until I looked at where it was added [1]. If present mainFrameImpl, was used again at line 779. Thus, I think it was just to provide access (and ensure it was not destroyed until that access). I don't think this is responsible for managing the lifetime anymore.
> 
> [1] http://trac.webkit.org/browser/trunk/WebKit/chromium/src/WebViewImpl.cpp?rev=50739#L761

Oh, it seems you're right.
However this code removal isn't related to this bug.  I recommend to remove it in a separated patch.

[Kent Tamura]

(In reply to comment #5)
> > For WebPagePopupImpl, I think m_page was necessary until close() because it received events. Now it has m_closing flag and we may destroy m_page in closePopup.
> 
> Are you saying that m_closing should protect us or that destroying m_page in closePopup() is a problem? Should I add checks to animate(), layout(), and paint()?

I meant destroying m_page in closePopup() was a problem before introducing m_closing. You can go ahead the patch now.  We don't need m_page check in animate, layout, and paint because they already check it in PageWidgetDelegate.

[David Dorwin]

Created attachment 177577 [details]
Patch

[David Dorwin]

Comment on attachment 177370 [details]
Patch

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

>>>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:142
>>>> +    deinitPage();
>>> 
>>> nit: The word 'deinit' is not popular in WebKit.  We had better rename it to destroyPage, destructPage, or closePage?
>>> 
>>> Note that we should have given full-spelled names to 'init' and 'initPage'.
>> 
>> Okay, I will change it. Do you have a preference for destroy vs. close? Shall I also change init to initialize/create or is that too far outside the scope of this CL? (The naming may depend on whether we actually destroy m_page.)
> 
> I prefer destoryPage.
> 
> I don't think you should rename init/initPage in this bug.

Done.

>>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:217
>>> +    // Initiate shutdown. This will cause a lot of notifications to be sent.
>> 
>> I guess this comment explained a reason of RefPtr<WebFrameImpl>, and we don't need the comment in this case.
> 
> I'm not sure it was related to that (see my comment in WebViewImpl.cpp below). I'll delete it anyway.

Done.

>>>> Source/WebKit/chromium/src/WebViewImpl.cpp:-1536
>>>> -            mainFrameImpl = WebFrameImpl::fromFrame(m_page->mainFrame());
>>> 
>>> I'm not sure if this is really unnecessary.
>> 
>> I was unsure as well until I looked at where it was added [1]. If present mainFrameImpl, was used again at line 779. Thus, I think it was just to provide access (and ensure it was not destroyed until that access). I don't think this is responsible for managing the lifetime anymore.
>> 
>> [1] http://trac.webkit.org/browser/trunk/WebKit/chromium/src/WebViewImpl.cpp?rev=50739#L761
> 
> Oh, it seems you're right.
> However this code removal isn't related to this bug.  I recommend to remove it in a separated patch.

Done - Removed file.

[Kent Tamura]

Comment on attachment 177577 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 177577 [details]
Patch

Clearing flags on attachment: 177577

Committed r136631: <http://trac.webkit.org/changeset/136631>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.