Make sure that ServiceWorkerFrameLoaderClient lifetime exceeds its frame lifetime
<rdar://problem/60187332>
Created attachment 393134 [details] Patch
Comment on attachment 393134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393134&action=review > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:92 > +void ServiceWorkerFrameLoaderClient::frameLoaderDestroyed() This seems like code that is bound to cause leaks, error prone at the very least. If you call ServiceWorkerFrameLoaderClient::create() and do nothing, it leaks. Not a great pattern.
(In reply to Chris Dumez from comment #3) > Comment on attachment 393134 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393134&action=review > > > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:92 > > +void ServiceWorkerFrameLoaderClient::frameLoaderDestroyed() > > This seems like code that is bound to cause leaks, error prone at the very > least. If you call ServiceWorkerFrameLoaderClient::create() and do nothing, > it leaks. Not a great pattern. Agreed. This is the current pattern for all WebFrameLoaderClient in WebKit1 and WebKit2. I thought of making Frame take a UniqueRef<FrameLoaderClient> so that we do not have this pattern anymore but ended up more conservative in this patch to specifically fix this crash. Refactoring could be done as a follow-up.
Comment on attachment 393134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393134&action=review >>> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:92 >>> +void ServiceWorkerFrameLoaderClient::frameLoaderDestroyed() >> >> This seems like code that is bound to cause leaks, error prone at the very least. If you call ServiceWorkerFrameLoaderClient::create() and do nothing, it leaks. Not a great pattern. > > Agreed. > This is the current pattern for all WebFrameLoaderClient in WebKit1 and WebKit2. > I thought of making Frame take a UniqueRef<FrameLoaderClient> so that we do not have this pattern anymore but ended up more conservative in this patch to specifically fix this crash. Refactoring could be done as a follow-up. And in some cases, we just leak new WebFrameLoaderClient!
Comment on attachment 393134 [details] Patch r=me Seems right to me that we should fix the bug and then change the design, so I guess I'll say r+. The ownership design for WebFrameLoaderClient seems busted and worth fixing, though. I'd prefer a design where FrameLoader kept a unique_ptr to the client, or, if that's not possible, then a RefPtr.
Comment on attachment 393134 [details] Patch Clearing flags on attachment: 393134 Committed r258223: <https://trac.webkit.org/changeset/258223>
All reviewed patches have been landed. Closing bug.