We relay on willDestroyFrame callback of WKBundlePageLoaderClient, but this callback is never emitted and can't be emitted. It was my fault, I thought I had tested it when I added it, but the thing is that willDestroyFrame is called from WebFrameLoaderClient::frameLoaderDestroyed(), but at that point the frame has already been detached from the page, so WebFrame::page() always returns nullptr at that point. But even if we had a WebPage pointer at that time, when the page is closed, the loader client is reset before FrameLoader::detachFromParent() si called, so when WebFrameLoaderClient::frameLoaderDestroyed() is called then loader client doesn't have callbacks attached.
Created attachment 246647 [details] Patch
Comment on attachment 246647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246647&action=review > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:81 > +class WebKitFrameWrapper final: public FrameDestructionObserver { I wonder if it makes sense to simply add the FrameDestructionObserver as a member of WebKitFrame? I worry about the possibility of introducing reference cycles, by wrapping the frame itself. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:123 > +static void webkitFrameDestroy(WebFrame* webFrame) > +{ > + webFrameMap().remove(webFrame); This should probably be called webkitFrameDestroyed or webkitFrameDestroyedCallback, I think. It doesn't destroy the frame, it just responds to the destruction.
(In reply to comment #2) > Comment on attachment 246647 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246647&action=review > > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:81 > > +class WebKitFrameWrapper final: public FrameDestructionObserver { > > I wonder if it makes sense to simply add the FrameDestructionObserver as a > member of WebKitFrame? I worry about the possibility of introducing > reference cycles, by wrapping the frame itself. There's no possibility of ref cycles, because the only reference of WebKitFrame is owned by the wrapper that is deleted when the WebCore::Frame is destroyed. That hasn't actually changed, since current the only references is owned by the WebFrameMap. The only difference is that now the WebFrameMap contains a WebKitFrameWrapper instead of a WebKitFrame directly. The destructor is in WebKitWebPage because frames are created in WebKitWebPage, which is where the WebFrameMap is. > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:123 > > +static void webkitFrameDestroy(WebFrame* webFrame) > > +{ > > + webFrameMap().remove(webFrame); > > This should probably be called webkitFrameDestroyed or > webkitFrameDestroyedCallback, I think. It doesn't destroy the frame, it just > responds to the destruction. It's confusing, it responds to a WebCore::Frame destruction to destroy the WebKitFrame object. But I agree it's confusing, webFrameDestroyed() could work better.
Comment on attachment 246647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246647&action=review >>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:123 >>> + webFrameMap().remove(webFrame); >> >> This should probably be called webkitFrameDestroyed or webkitFrameDestroyedCallback, I think. It doesn't destroy the frame, it just responds to the destruction. > > It's confusing, it responds to a WebCore::Frame destruction to destroy the WebKitFrame object. But I agree it's confusing, webFrameDestroyed() could work better. webFrameDestroyed sounds like a good name as well.
(In reply to comment #4) > Comment on attachment 246647 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246647&action=review > > >>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:123 > >>> + webFrameMap().remove(webFrame); > >> > >> This should probably be called webkitFrameDestroyed or webkitFrameDestroyedCallback, I think. It doesn't destroy the frame, it just responds to the destruction. > > > > It's confusing, it responds to a WebCore::Frame destruction to destroy the WebKitFrame object. But I agree it's confusing, webFrameDestroyed() could work better. > > webFrameDestroyed sounds like a good name as well. Ok, that's what I proposed, so any other concern?
Comment on attachment 246647 [details] Patch No, this looks okay.
(In reply to comment #6) > Comment on attachment 246647 [details] > Patch > > No, this looks okay. Thank you!
Committed r180211: <http://trac.webkit.org/changeset/180211>