| Summary: | Make sure that ServiceWorkerFrameLoaderClient lifetime exceeds its frame lifetime | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||
| Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | achristensen, cdumez, commit-queue, ggaren, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
youenn fablet
2020-03-10 04:11:30 PDT
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. |