WebPageProxy::setAppHighlightsVisibility might send message from a background thread, ASSERTing

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

WebPageProxy::setAppHighlightsVisibility might send message from a background thread, ASSERTing

<rdar://80056481>

[Brady Eidson]

Created attachment 432765 [details]
Patch

[Chris Dumez]

Can’t we just send the radar to the client? I worry about us opening the door to clients calling ours APIs on non main threads.

[Brady Eidson]

(In reply to Chris Dumez from comment #2)
> Can’t we just send the radar to the client? I worry about us opening the
> door to clients calling ours APIs on non main threads.

The client is *US*

[Brady Eidson]

(In reply to Brady Eidson from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Can’t we just send the radar to the client? I worry about us opening the
> > door to clients calling ours APIs on non main threads.
> 
> The client is *US*

(It's not really us, but it's our own block we're passing to another system framework and it's expected that block gets called on a non-main thread)

[Brady Eidson]

(In reply to Brady Eidson from comment #4)
> (In reply to Brady Eidson from comment #3)
> > (In reply to Chris Dumez from comment #2)
> > > Can’t we just send the radar to the client? I worry about us opening the
> > > door to clients calling ours APIs on non main threads.
> > 
> > The client is *US*
> 
> (It's not really us, but it's our own block we're passing to another system
> framework and it's expected that block gets called on a non-main thread)

So, if you prefer, I could fix this inside that block instead.

[Chris Dumez]

(In reply to Brady Eidson from comment #5)
> (In reply to Brady Eidson from comment #4)
> > (In reply to Brady Eidson from comment #3)
> > > (In reply to Chris Dumez from comment #2)
> > > > Can’t we just send the radar to the client? I worry about us opening the
> > > > door to clients calling ours APIs on non main threads.
> > > 
> > > The client is *US*
> > 
> > (It's not really us, but it's our own block we're passing to another system
> > framework and it's expected that block gets called on a non-main thread)
> 
> So, if you prefer, I could fix this inside that block instead.

Oh, I see. Then yes, I think it would look nicer to fix it at the call site rather than doing a trampoline in WebPageProxy.

[Chris Dumez]

Comment on attachment 432765 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:591
> +        callOnMainRunLoop([protectedThis = makeRef(*this), appHighlightsVisibility] {

As I mentioned earlier, I think this should be fixed at the call site.

Also, I looked at the call site in WebPageProxy::setUpHighlightsObserver() and I am not convinced this is safe. What ensures that setAppHighlightsVisibility() is not getting called on the background thread while the WebPageProxy is in the middle of destruction on the main thread?
Your call to makeRef(*this) here would try to ref the page even though its destructor is running. Is there something I missed that makes sure this cannot happen?

[Tim Horton]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 432765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432765&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:591
> > +        callOnMainRunLoop([protectedThis = makeRef(*this), appHighlightsVisibility] {
> 
> As I mentioned earlier, I think this should be fixed at the call site.

I said the same to Brady elsewhere and he promised to make that change (and then I r+’d anyway :| )

[Brady Eidson]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 432765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432765&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:591
> > +        callOnMainRunLoop([protectedThis = makeRef(*this), appHighlightsVisibility] {
> 
> As I mentioned earlier, I think this should be fixed at the call site.
> 
> Also, I looked at the call site in WebPageProxy::setUpHighlightsObserver()
> and I am not convinced this is safe. What ensures that
> setAppHighlightsVisibility() is not getting called on the background thread
> while the WebPageProxy is in the middle of destruction on the main thread?
> Your call to makeRef(*this) here would try to ref the page even though its
> destructor is running. Is there something I missed that makes sure this
> cannot happen?

Right.

Turns out this code is already thread unsafe.

Fixing now with some weakptr and Function<>-foo

[Brady Eidson]

Created attachment 432803 [details]
Patch reviewed on Slack

[EWS]

Committed r279509 (239361@main): <https://commits.webkit.org/239361@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432803 [details].