Bug 120401 – [GTK][WTR] Several editing/selection tests are flaky as ClickCounter is not reset between test runs

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[Simon Pena]

WebKitWebViewBase has a ClickCounter helper class to handle single-clicks, double-clicks, triple-clicks and so on: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/gtk/GtkClickCounter.h

WebKitTestRunner uses EventSenderProxy to create and dispatch GdkEvents with the mouse event. In TestController::resetStateToConsistentValues(), the EventSenderProxy is destroyed after every test run, which should clear existing click counts, previous click locations and other mouse event related state. However, since the internal ClickCounter is not cleared, and as it is used in webkitWebViewBaseButtonPressEvent to create NativeWebMouseEvent from the GdkEvents received, these new NativeWebMouseEvents carry the existing click count.

The tests are simply flaky: when they are run in their own WebKitTestRunner, they will pass. This shouldn't affect the regular browsing behaviour, as the ClickCounter uses the event time and click location to tell whether a click is a different event or part of the previous one. However, I still think it should be fixed, as it can be misleading when analysing test results.

I will now upload a patch that resets the click counter in the resetStateToConsistentValues, and also in the WEBKIT_LOAD_STARTED state. Let me know what you think.

You can try this yourself by running 

Tools/Scripts/run-webkit-tests --repeat-each=2 --gtk -2 editing/selection/anchor-focus1.html \
editing/selection/anchor-focus2.html \
editing/selection/anchor-focus3.html \
editing/selection/block-with-positioned-lastchild.html \
editing/selection/caret-after-keypress.html \
editing/selection/crash-on-shift-click.html \
editing/selection/doubleclick-beside-cr-span.html \
editing/selection/doubleclick-crash.html \
editing/selection/doubleclick-inline-first-last-contenteditable.html \
editing/selection/doubleclick-whitespace.html \
editing/selection/drag-select-1.html \
editing/selection/drag-select-rapidly.html \
editing/selection/drag-start-event-client-x-y.html \
editing/selection/editable-links.html \
editing/selection/expanding-selections.html \
editing/selection/expanding-selections2.html \
editing/selection/fake-doubleclick.html \
editing/selection/fake-drag.html \
editing/selection/focus-and-display-none.html \
editing/selection/focus-crash.html \
editing/selection/hit-test-anonymous.html \
editing/selection/hit-test-on-text-with-line-height.html \
editing/selection/inline-closest-leaf-child.html \
editing/selection/last-empty-inline.html \
editing/selection/mixed-editability-1.html \
editing/selection/move-begin-end.html \
editing/selection/paragraph-granularity.html \
editing/selection/rtl-move-selection-right-left.html \
editing/selection/select-across-readonly-input-1.html \
editing/selection/select-across-readonly-input-2.html \
editing/selection/select-across-readonly-input-3.html \
editing/selection/select-across-readonly-input-4.html \
editing/selection/select-across-readonly-input-5.html \
editing/selection/select-all-iframe.html \
editing/selection/select-bidi-run.html \
editing/selection/select-from-textfield-outwards.html \
editing/selection/select-line-break-with-opposite-directionality.html \
editing/selection/select-out-of-editable.html \
editing/selection/select-out-of-floated-contenteditable.html \
editing/selection/select-out-of-floated-input.html \
editing/selection/select-out-of-floated-textarea.html \
editing/selection/selection-actions.html

I took this list from http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20WK2%20%28Tests%29/builds/7907/steps/layout-test/logs/stdio. I removed editing/selection/empty-cell-right-click.html, which seems to cause a different issue and that I am investigating on its own.

[Simon Pena]

Created attachment 209863 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 209863 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1472
> +        // Reset the click counter
> +        webkitWebViewBaseResetClickCounter(WEBKIT_WEB_VIEW_BASE(webView));

This is probably not the right time to do this. Just because a new load started doesn't mean the old page isn't active. Maybe a better time would be after the document changes. It might be hard to track this in the UI process, so perhaps it's better to wait for the first repaint that includes the new page. This should probably also be in WebKitWebViewBase.

[Simon Pena]

(In reply to comment #3)
> (From update of attachment 209863 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209863&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1472
> > +        // Reset the click counter
> > +        webkitWebViewBaseResetClickCounter(WEBKIT_WEB_VIEW_BASE(webView));
> 
> This is probably not the right time to do this. Just because a new load started doesn't mean the old page isn't active. Maybe a better time would be after the document changes. It might be hard to track this in the UI process, so perhaps it's better to wait for the first repaint that includes the new page. This should probably also be in WebKitWebViewBase.

As I explain in comment #0, that is probably the less important change: in the regular browsing, the ClickCounter is really unlikely to cause a wrong behaviour. And when WebKitTestRunner is used, webkitWebViewLoadChanged is never reached. So I wonder what's your opinion on the other changes in the patch: do you think that could be a good approach? (Of course, I will address your comment as well)

[Simon Pena]

Created attachment 211757 [details]
Patch

Removed the call to reset ClickCounter from the WebKitWebView

[Manuel Rego Casasnovas]

*** Bug 121952 has been marked as a duplicate of this bug. ***

[Simon Pena]

OK: I've talked to Martin, and he agrees to have this split into two issues. We would file a new bug for the issue in the general case, and land this fix for the layout tests.

[Martin Robinson]

Comment on attachment 211757 [details]
Patch

Looks good to me, but I think a WK2 owner needs to verify the changes to platform independent code.

[Simon Pena]

(In reply to comment #8)
> (From update of attachment 211757 [details])
> Looks good to me, but I think a WK2 owner needs to verify the changes to platform independent code.

Sam, Alexey, could any of you take a look at the changes in Tools/WebKitTestRunner/PlatformWebView.h and Tools/WebKitTestRunner/TestController.cpp? Thanks!

[Alexey Proskuryakov]

Could you please clarify why this is a responsibility of WKTR? On the face of it, it seems that the counter should be reset during regular page navigations, so the fix would be in WebKit2/gtk.

[Simon Pena]

(In reply to comment #10)
> Could you please clarify why this is a responsibility of WKTR? On the face of it, it seems that the counter should be reset during regular page navigations, so the fix would be in WebKit2/gtk.

You are right: the proper fix would be in WebKit2/GTK (we filed bug #122551 about it), but I thought of this as a partial fix that would get the layout tests working properly again.

Fixing it in WebKit2/GTK was the first approach I tried -unsuccessfully-, but I am not sure where exactly it makes sense to proceed and reset it (see also Martin's comment #3). I don't mind trying again, but would appreciate some suggestions about where/when to handle this and reset the counter.

[Alexey Proskuryakov]

Comment on attachment 211757 [details]
Patch

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

I'm OK with landing this change.

> Tools/WebKitTestRunner/TestController.cpp:635
> +#if PLATFORM(GTK)
> +    m_mainWebView->resetStateToConsistentValues();
> +#endif

Perhaps this should be named less generally if we don't expect more code in the reset function - and we don't.

I'd just call this resetGtkClickCounter(), and add a FIXME pointing to bug 122551.

[Martin Robinson]

We need to get the tests running, so let's just land this and figure out a better way to fix it later.

[Martin Robinson]

Comment on attachment 211757 [details]
Patch

This also needs to unskip the tests.

[Alexey Proskuryakov]

Why is this needed, given that bug 122551 is already fixed? That bug was supposed to fix this for real, as opposed to this workaround.

[Simon Pena]

I will try to talk to Quique, who fixed bug #122551, and see if this patch is still needed.

[Simon Pena]

I have just talked to Quique and he showed me https://bugs.webkit.org/show_bug.cgi?id=122551#c7. According to that comment, his patch in bug #122551 addressed this problem in the proper way, so the patch here shouldn't be needed any more.

I will still try to see if the patch here makes any difference: if it doesn't, we can clear the flags and close it.

[Martin Robinson]

It look like the patch for 122551 did not actually unskip the tests.

*** This bug has been marked as a duplicate of bug 122551 ***

[Simon Pena]

Comment on attachment 211757 [details]
Patch

Clearing the flags.