dispatchViewStateChange should not wait for sync reply if the page isn't visible

• Component: WebKit2
• Status: RESOLVED FIXED

[Gavin Barraclough]

This is particularly problematic on iOS, since if the page isn't visible the process is likely suspended.

<rdar://problem/20967937>

[Gavin Barraclough]

Created attachment 253496 [details]
Fix

[Gavin Barraclough]

Fixed in r184692

[Alexey Proskuryakov]

Comment on attachment 253496 [details]
Fix

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1377
> +    if (!(m_viewState & ViewState::IsVisible))
> +        m_viewStateChangeWantsSynchronousReply = false;

The initial m_viewState is invisible, so doesn't this patch change behavior even for pages that are actually visible?

This seems very dangerous.

[Gavin Barraclough]

(In reply to comment #3)
> Comment on attachment 253496 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253496&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1377
> > +    if (!(m_viewState & ViewState::IsVisible))
> > +        m_viewStateChangeWantsSynchronousReply = false;
> 
> The initial m_viewState is invisible, so doesn't this patch change behavior
> even for pages that are actually visible?

I believe we were only doing the sync wait when the app reactivated, in which case the visible view was already in the hierarchy & marked visible, so the sync update isn't suppressed.
 
> This seems very dangerous.

We shouldn't ever need to synchronously change the view state while not visible. If the act on changing the view from invisible to visible needed to be synchronous, then that call to set the view state needed to be the one marked to be synchronous. If we were ever failing to do so, then that was a bug that could cause problems; we should find it and fix it. This is bad behavior, and we shouldn't keep it to try to paper over other hypothetical mistakes.

[Alexey Proskuryakov]

> I believe we were only doing the sync wait when the app reactivated, in which case the visible view was already in the hierarchy & marked visible, so the sync update isn't suppressed.

Ok.

> we should find it and fix it

What I'm saying is that this change is more dangerous than it seems, so it should be tested proactively and thoroughly. There is very little difference between uncovering a pre-existing bug, and creating a new one.