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.
Created attachment 245044 [details] Patch
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
Created attachment 245048 [details] Patch
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!
Created attachment 245246 [details] Patch
Created attachment 245248 [details] Patch
(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?
Is this patch still valid ?
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.
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.