Summary: | [GTK] WebKitFrame objects are never released | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | gustavo, svillar, zan | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 141558 | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2015-02-16 05:08:32 PST
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> |