Bug 140713 – [WK2] willDestroyGlobalObjectForDOMWindowExtension callback which registered in injectedBundle is not called while WebPage is being closed

• Component: WebKit2
• Status: RESOLVED INVALID

[Joonghun Park]

When using WKBundleDOMWindowExtension in InjectedBundle, willDestroyGlobalObjectForDOMWindowExtension is not called 
because InjectedBundlePageLoaderClient's initialization in WebPage::close() is called too early, namely, before setting the frame's m_view to 0;
So move initialize(0) calls to WebPage's m_*client group to the end of WebPage::close() function to correct such problems.

[Joonghun Park]

Created attachment 245044 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 245044 [details]
Patch

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

This patch should be reviewed by WK2 owner.

> Source/WebKit2/ChangeLog:10
> +        So move initialize(0) calls to WebPage's m_*client group to the end of WebPage::close() function 

typo: m_*client -> m_client

[Joonghun Park]

Created attachment 245048 [details]
Patch

[Anders Carlsson]

Comment on attachment 245048 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1024
> +    // The WebPage can be destroyed by this call.
> +    WebProcess::shared().removeWebPage(m_pageID);

This can't be moved here - this call will destroy the WebPage object whose members are accessed later!

[Joonghun Park]

Created attachment 245246 [details]
Patch

[Joonghun Park]

Created attachment 245248 [details]
Patch

[Joonghun Park]

(In reply to comment #4)
> Comment on attachment 245048 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245048&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1024
> > +    // The WebPage can be destroyed by this call.
> > +    WebProcess::shared().removeWebPage(m_pageID);
> 
> This can't be moved here - this call will destroy the WebPage object whose
> members are accessed later!

Could you please review the change which I revised?

[Gyuyoung Kim]

Is this patch still valid ?

[Darin Adler]

Hard to say, but:

1) We need a regression test for this, too, not just a bug fix.

2) There’s risk to reordering that destruction sequence, so we have to make sure we have done sufficient testing of what other effects the change has.

[Brady Eidson]

Comment on attachment 245248 [details]
Patch

This patch has been pending review since 2015 with no recent activity.
It seems unlikely that it would even still apply to trunk in its current form.

(Also it hasn't been updated for Darin's feedback)

Clearing from the review queue.

Feel free to update and resubmit if the patch is still relevant.