Bug 195223 – Add a WebViewDidMoveToWindowObserver for WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Add a WebViewDidMoveToWindowObserver for WKWebView.

[Radar WebKit Bug Importer]

<rdar://problem/48520161>

[Jiewen Tan]

Created attachment 363365 [details]
Patch

[Darin Adler]

Comment on attachment 363365 [details]
Patch

What is the "load optimizer"?

[Darin Adler]

Comment on attachment 363365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363365&action=review

> Source/WebKit/UIProcess/WebPageProxy.h:1467
> +    void addWebViewDidMoveToWindowObserver(WebViewDidMoveToWindowObserver&);
> +    void removeWebViewDidMoveToWindowObserver(WebViewDidMoveToWindowObserver*);

I think these functions could have shorter names. C++ overloading means they could have names as short as "add/remove", but I was thinking "addObserver/removeObserver".

Can the remove function take a reference instead of a pointer? Is there a caller that needs to call it with null?

> Source/WebKit/UIProcess/WebPageProxy.h:2389
> +    HashMap<WebViewDidMoveToWindowObserver*, WeakPtr<WebViewDidMoveToWindowObserver>> m_webViewDidMoveToWindowObservers;

Is a WeakPtr the optimal pattern here? Where else are we using that for observers?

Since this entire observer function is just a "call this function" thing, maybe we could do something with functions instead? Unless this is a common pattern.

[Jiewen Tan]

Comment on attachment 363365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363365&action=review

Thanks Darin for r+ the patch.

>> Source/WebKit/UIProcess/WebPageProxy.h:1467
>> +    void removeWebViewDidMoveToWindowObserver(WebViewDidMoveToWindowObserver*);
> 
> I think these functions could have shorter names. C++ overloading means they could have names as short as "add/remove", but I was thinking "addObserver/removeObserver".
> 
> Can the remove function take a reference instead of a pointer? Is there a caller that needs to call it with null?

Fixed.

>> Source/WebKit/UIProcess/WebPageProxy.h:2389
>> +    HashMap<WebViewDidMoveToWindowObserver*, WeakPtr<WebViewDidMoveToWindowObserver>> m_webViewDidMoveToWindowObservers;
> 
> Is a WeakPtr the optimal pattern here? Where else are we using that for observers?
> 
> Since this entire observer function is just a "call this function" thing, maybe we could do something with functions instead? Unless this is a common pattern.

A more common pattern is to use HashSet<WebViewDidMoveToWindowObserver*> without weak ptrs. However, that relies on the observers to remove themselves from the set when they are about to destroy. Otherwise, UAF could happen.

I could see pros and cons of using Functions here.
Pros: More clean code; Leave UAF from the WebPageProxy to Functions suppliers.
Cons: More memory.

I don't really have opinions on which is significantly better than which. It looks like that it is a bigger topic to discuss than this patch. If you don't have strong opinions with my current approach, I will go ahead to land this patch. Maybe we could open a discussion on [e-mail]?

[Jiewen Tan]

Created attachment 363478 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363478 [details]
Patch for landing

Clearing flags on attachment: 363478

Committed r242336: <https://trac.webkit.org/changeset/242336>

[Jiewen Tan]

A follow up from Tim's comment is committed:
Committed r242398: <https://trac.webkit.org/changeset/242398>