Bug 208855 – Make sure that ServiceWorkerFrameLoaderClient lifetime exceeds its frame lifetime

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Make sure that ServiceWorkerFrameLoaderClient lifetime exceeds its frame lifetime

[youenn fablet]

<rdar://problem/60187332>

[youenn fablet]

Created attachment 393134 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

(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.

[Geoffrey Garen]

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!

[Geoffrey Garen]

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.

[WebKit Commit Bot]

Comment on attachment 393134 [details]
Patch

Clearing flags on attachment: 393134

Committed r258223: <https://trac.webkit.org/changeset/258223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.