Bug 101753 – [WK2] TiledBackingStore: User events are sent to web page before it is shown

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

To see the problem:
1) go to maps.google.com from any other page
2) start move mouse over the upper part of the viewport while maps.google.com hasn't turned up
3) See "Maps" balloon that should not be there yet.

[Mikhail Pozdnyakov]

Created attachment 175223 [details]
WIP patch

[Mikhail Pozdnyakov]

Created attachment 175228 [details]
patch

[Yael]

I think the problem statement is not correct.
The bug is not that user events are sent to the page before it is showing.
The real bug is that it takes so much time to show the page after it was loaded and layout has finished.

[Mikhail Pozdnyakov]

(In reply to comment #3)
> I think the problem statement is not correct.
> The bug is not that user events are sent to the page before it is showing.
> The real bug is that it takes so much time to show the page after it was loaded and layout has finished.
Off course, but even if the delay were not that obvious I think there would still some raise condition, so that unlucky user (lucky tester ;-) ) is able to interact with a page which is not yet fully in place..

[Yael]

(In reply to comment #4)
> (In reply to comment #3)
> > I think the problem statement is not correct.
> > The bug is not that user events are sent to the page before it is showing.
> > The real bug is that it takes so much time to show the page after it was loaded and layout has finished.
> Off course, but even if the delay were not that obvious I think there would still some raise condition, so that unlucky user (lucky tester ;-) ) is able to interact with a page which is not yet fully in place..
If you can interact with the page, it means that hit test was successful, which means that the page IS there.
If the page is not there yet, you cannot interact with it.

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > I think the problem statement is not correct.
> > > The bug is not that user events are sent to the page before it is showing.
> > > The real bug is that it takes so much time to show the page after it was loaded and layout has finished.
> > Off course, but even if the delay were not that obvious I think there would still some raise condition, so that unlucky user (lucky tester ;-) ) is able to interact with a page which is not yet fully in place..
> If you can interact with the page, it means that hit test was successful, which means that the page IS there.
> If the page is not there yet, you cannot interact with it.
From the User perspective it's not there until it is rendered.

[Yael]

Let me try again: we should fix the huge delay that we have in painting and not put band-aid on it.

[Kenneth Rohde Christiansen]

This is not bandaiding. There is another issue, right, but a document is created and can accept events before the initial layout is complete.

[Kenneth Rohde Christiansen]

Comment on attachment 175228 [details]
patch

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

> Source/WebKit2/UIProcess/WebPageProxy.h:1245
> +    bool m_suppressUserEvents;

Isn't a m_isPageTransitionFinished more descriptive?

[Kenneth Rohde Christiansen]

We had similar issues on the N9. Other cases include not changing contents while the user is still interacting with the page, ie doing pinch, pan or similar. This should also postpose orientation change.

[Rafael Brandao]

Comment on attachment 175228 [details]
patch

This is affecting ports that use TILED_BACKING_STORE, so you should remove [EFL] as it's not EFL-specific. Maybe there's a flag for tiled backing store?

[Rafael Brandao]

Also, is there anything we could do to test this? Maybe a layout test?

[Andras Becsi]

Since this also affects other ports I think the [EFL] tag should be dropped from the title.

[Mikhail Pozdnyakov]

(In reply to comment #11)
> (From update of attachment 175228 [details])
> This is affecting ports that use TILED_BACKING_STORE, so you should remove [EFL] as it's not EFL-specific. Maybe there's a flag for tiled backing store?

Indeed. Completely forgot about title. Thanks!

[Mikhail Pozdnyakov]

(In reply to comment #12)
> Also, is there anything we could do to test this? Maybe a layout test?

Think it is hard to do reliable layout test for this issue as on JS side there is no way to get information about when the layout is actually complete (correct me if I'm wrong). And the time of the delay between document creation and layout completion may very (should be as short as possible :) ).

[Noam Rosenthal]

(In reply to comment #15)
> (In reply to comment #12)
> > Also, is there anything we could do to test this? Maybe a layout test?
> 
> Think it is hard to do reliable layout test for this issue as on JS side there is no way to get information about when the layout is actually complete (correct me if I'm wrong). And the time of the delay between document creation and layout completion may very (should be as short as possible :) ).

We should find a way to add didLayout to pageLoaderClient in WTR.

[Sam Weinig]

Comment on attachment 175228 [details]
patch

Please don't add all these #ifdefs in the middle of WebPage like this.

[Jocelyn Turcotte]

Comment on attachment 175228 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3348
>  void WebPage::commitPageTransitionViewport()
>  {
>      m_drawingArea->setLayerTreeStateIsFrozen(false);
> +    send(Messages::WebPageProxy::PageTransitionFinished());

This is a UI process message handler and as a result you're sending back a message with a different meaning unconditionally. I don't think it's necessary to send those messages back and forth and you're actually going to cause more traffic between processes than what you save by not sending user events during that short lapse.
In other words: you could discard those events directly in the web process or use information that the UI process already have instead to switch it on/off.

[Mikhail Pozdnyakov]

(In reply to comment #18)
> (From update of attachment 175228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175228&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3348
> >  void WebPage::commitPageTransitionViewport()
> >  {
> >      m_drawingArea->setLayerTreeStateIsFrozen(false);
> > +    send(Messages::WebPageProxy::PageTransitionFinished());
> 
> This is a UI process message handler and as a result you're sending back a message with a different meaning unconditionally. I don't think it's necessary to send those messages back and forth and you're actually going to cause more traffic between processes than what you save by not sending user events during that short lapse.
> In other words: you could discard those events directly in the web process or use information that the UI process already have instead to switch it on/off.

If we assume that time delta is short enough, so that the User is not able to initiate a lot of events (now s/he can, but hopefully the delay will be fixed) than yes, I agree with you. 

Will prepare a patch based on new approach.

[Mikhail Pozdnyakov]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 175228 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=175228&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3348
> > >  void WebPage::commitPageTransitionViewport()
> > >  {
> > >      m_drawingArea->setLayerTreeStateIsFrozen(false);
> > > +    send(Messages::WebPageProxy::PageTransitionFinished());
> > 
> > This is a UI process message handler and as a result you're sending back a message with a different meaning unconditionally. I don't think it's necessary to send those messages back and forth and you're actually going to cause more traffic between processes than what you save by not sending user events during that short lapse.
> > In other words: you could discard those events directly in the web process or use information that the UI process already have instead to switch it on/off.
> 
> If we assume that time delta is short enough, so that the User is not able to initiate a lot of events (now s/he can, but hopefully the delay will be fixed) than yes, I agree with you. 
> 
> Will prepare a patch based on new approach.
Now I see that it is not that good idea as web process sends message back to UI process once it handles an event and UI process is relying on it, so think that ignoring of user events on web process side will affect the existing logic.

[Jocelyn Turcotte]

(In reply to comment #20)
> Now I see that it is not that good idea as web process sends message back to UI process once it handles an event and UI process is relying on it, so think that ignoring of user events on web process side will affect the existing logic.

Couldn't you send them back as unhandled all the time instead of ignoring them?

I'm not sure if I'm following, since it seems like doing an early return in WebPageProxy::handleTouchEvent as your previous patch does would cause the same issue.

[Mikhail Pozdnyakov]

Created attachment 175432 [details]
patch v2

Took comments from Sam and Kenneth into consideration.

[Mikhail Pozdnyakov]

(In reply to comment #21)
> (In reply to comment #20)
> > Now I see that it is not that good idea as web process sends message back to UI process once it handles an event and UI process is relying on it, so think that ignoring of user events on web process side will affect the existing logic.
> 
> Couldn't you send them back as unhandled all the time instead of ignoring them?

This should be ok.
> 
> I'm not sure if I'm following, since it seems like doing an early return in WebPageProxy::handleTouchEvent as your previous patch does would cause the same issue.

It wouldn't cause the same issue as it returns at very beginning when neither UI process nor WEB process states are changed.

[Mikhail Pozdnyakov]

Created attachment 175459 [details]
patch v3

[Mikhail Pozdnyakov]

Comment on attachment 175459 [details]
patch v3

Events are suppressed on WEB process side as proposed by Jocelyn.

[Kenneth Rohde Christiansen]

Comment on attachment 175459 [details]
patch v3

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1753
>  void WebPage::touchEvent(const WebTouchEvent& touchEvent)
>  {
> +    if (sendIfUserEventCannotBeHandled(touchEvent))
> +        return;
> +
>      CurrentEvent currentEvent(touchEvent);
>  
>      bool handled = handleTouchEvent(touchEvent, m_page.get());

send(Messages::WebPageProxy::DidReceiveEvent(static_cast<uint32_t>(touchEvent.type()), handled));
}

I think it is weird that the sendIfUserEventCannotBeHandled does the above but in a kind of non transparent way.

Why not 

bool handled = false;

if (canHandleEvent()) {
    CurrentEvent currentEvent(touchEvent);
    handled = handleTouchEvent(touchEvent, m_page.get());
}

send(Messages::WebPageProxy::DidReceiveEvent(static_cast<uint32_t>(touchEvent.type()), handled));

[Rafael Brandao]

Comment on attachment 175459 [details]
patch v3

I wonder if it is the burden of WebPage to care when it should suppress or not those events. You could have not sent the event at all on the webview at the first place, right? You may need to add a way to detect when the first render is done (if it doesn't exist yet) and then you can turn the events on again on your client. My point is why this is a issue now for WK2, but not for other ports, like mac?

[Mikhail Pozdnyakov]

Created attachment 175656 [details]
patch v4

Took input from Kenneth into consideration. Thanks, Kenneth!

[Mikhail Pozdnyakov]

(In reply to comment #27)
> (From update of attachment 175459 [details])
> I wonder if it is the burden of WebPage to care when it should suppress or not those events. You could have not sent the event at all on the webview at the first place, right? You may need to add a way to detect when the first render is done (if it doesn't exist yet) and then you can turn the events on again on your client.

That would bring IPC overhead, see comment #18

[Rafael Brandao]

(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 175459 [details] [details])
> > I wonder if it is the burden of WebPage to care when it should suppress or not those events. You could have not sent the event at all on the webview at the first place, right? You may need to add a way to detect when the first render is done (if it doesn't exist yet) and then you can turn the events on again on your client.
> 
> That would bring IPC overhead, see comment #18

I don't see the overhead, it would be a single message for the client saying the first render happened. And then you could turn the suppressing events flag off right there on the client. I believe this kind of message is already there (layout milestones, see bug #95397), but I'm not sure if it fits into the problem you're trying to solve. I was just wondering if we are not going into the wrong direction here.

Does WK1 also ignore events until the first render is done?

[WebKit Review Bot]

Comment on attachment 175656 [details]
patch v4

Attachment 175656 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14967529

New failing tests:
transforms/3d/hit-testing/backface-hit-test.html
transforms/3d/hit-testing/backface-no-transform-hit-test.html

[Jocelyn Turcotte]

(In reply to comment #30)
> Does WK1 also ignore events until the first render is done?

The problem is that we're not showing the committed content to the UI process immediately like WK1 does since we need to know at which zoom scale to render the  page, which is handled in the UI process and depends on the laid out contents size.
During that time, the old page is shown to the user but the the newly committed page is handling events in the web process.

This is not a problem for WK1 since the FrameView knows already that it should render at scale 1, handles all this synchronously and present everything it has to the user. And while it does this it's not handling user events, so the only window where the user might start interacting with the new page is while the rendered content is sent to the UI process.

If we want to handle this in the UI process, this should be done in cooperation with the PageViewportController which handles this delay and knows when it would be fine to eat events instead of the WebPageProxy. The nasty part is that it currently has nothing to do with events and it would require us to handle this outside of cross-platform code. It would also require adding more asynchronous logic to the problem, which wouldn't make things any simpler.

[Mikhail Pozdnyakov]

(In reply to comment #31)
> (From update of attachment 175656 [details])
> Attachment 175656 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14967529
> 
> New failing tests:
> transforms/3d/hit-testing/backface-hit-test.html
> transforms/3d/hit-testing/backface-no-transform-hit-test.html

Has to be unrelated as patch concerns WK2. Could please anyone review it?

[Jocelyn Turcotte]

Comment on attachment 175656 [details]
patch v4

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1515
> +    if (canHandleUserEvents()) {
> +        if (m_pageOverlay) {
> +            // Let the page overlay handle the event.
> +            handled = m_pageOverlay->mouseEvent(mouseEvent);
> +        }

The page overlay isn't bound to individual pages lifetime and isn't affected by their transition, so I would let it handle events first anyhow.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1904
> +    return !m_drawingArea->layerTreeStateIsFrozen();

Please add a comment that this only should only apply if the tree was frozen by didStartPageTransition. This will be a problem is this state is used in other cases in the future, especially in the middle of user interactions.
Ideally we would have a separate boolean to block user inputs and avoid mixing things, but I guess this isn't needed now.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1945
> -        m_drawingArea->setLayerTreeStateIsFrozen(false);
> +    m_drawingArea->setLayerTreeStateIsFrozen(false);

That over-indentation is because this might be handled by the else inside the #ifdef.
So unless the style check complains, I would leave it like it was.

[Mikhail Pozdnyakov]

Created attachment 176724 [details]
patch v5

Took comments from Jocelyn into consideration. Thanks Jocelyn!

[WebKit Review Bot]

Comment on attachment 176724 [details]
patch v5

Clearing flags on attachment: 176724

Committed r136133: <http://trac.webkit.org/changeset/136133>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.