Bug 227607 - WebPageProxy::setAppHighlightsVisibility might send message from a background thread, ASSERTing
Summary: WebPageProxy::setAppHighlightsVisibility might send message from a background...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-01 20:10 PDT by Brady Eidson
Modified: 2021-07-02 10:57 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.64 KB, patch)
2021-07-01 20:13 PDT, Brady Eidson
thorton: review+
Details | Formatted Diff | Diff
Patch reviewed on Slack (2.13 KB, patch)
2021-07-02 09:26 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2021-07-01 20:10:52 PDT
WebPageProxy::setAppHighlightsVisibility might send message from a background thread, ASSERTing

<rdar://80056481>
Comment 1 Brady Eidson 2021-07-01 20:13:58 PDT
Created attachment 432765 [details]
Patch
Comment 2 Chris Dumez 2021-07-01 21:19:50 PDT
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.
Comment 3 Brady Eidson 2021-07-01 21:25:28 PDT
(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*
Comment 4 Brady Eidson 2021-07-01 21:27:58 PDT
(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)
Comment 5 Brady Eidson 2021-07-01 21:28:17 PDT
(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.
Comment 6 Chris Dumez 2021-07-01 21:47:09 PDT
(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.
Comment 7 Chris Dumez 2021-07-01 22:06:21 PDT
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?
Comment 8 Tim Horton 2021-07-01 22:45:04 PDT
(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 :| )
Comment 9 Brady Eidson 2021-07-02 09:13:30 PDT
(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
Comment 10 Brady Eidson 2021-07-02 09:26:14 PDT
Created attachment 432803 [details]
Patch reviewed on Slack
Comment 11 EWS 2021-07-02 10:57:19 PDT
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].