Bug 41061 – User's gesture initiated from select's onchange event can not be treated as user initiated in chromium

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Johnny(Jianning) Ding]

Please refer to http://code.google.com/p/chromium/issues/detail?id=47276 for the bug details.

After investigation, this bug was because chromium thought the popup was not user initiated.
In function:  bool ScriptController::processingUserGesture(DOMWrapperWorld*) const  (WebCore/binding/v8/ScriptController.cpp)

Chromium tests whether UserGestureIndicator::processingUserGesture() returns true before checking the current event object.  If not, it returns false to indicate  the disposition was not user initiated. 

However, in JSC (WebCore/binding/js/ScriptController.cp), it tests the current event object at first, if the event can be treated as user gesture,  JSC will treat the disposition as user initiated.

For why UserGestureIndicator::processingUserGesture() returns false (both JSC and V8) when current event is from select element (widget), it's because
now we set the UserGestureIndicator as DefinitelyUserGesture only in the event handlers written in event_handler.cpp which are called by webview. For the event handlers which are called by widget (derived from FrameScrollView), we didn't change UserGestureIndicator to DefinitelyUserGesture.  I think we probably  need to fix this behavior.

I will provide a patch to make sure the ScriptController::processingUserGesture follows the JSC's behavior.

[Johnny(Jianning) Ding]

Created attachment 59509 [details]
test case

[Johnny(Jianning) Ding]

Created attachment 59520 [details]
patch v1

[Adam Barth]

Comment on attachment 59520 [details]
patch v1

The JSC implementation of this function is 10 lines long.  Why is the V8 version 50 lines?  Please update the V8 version to match the JSC version.

[Johnny(Jianning) Ding]

The main reason for this issue is in chromium the select was implemented by a widget (derived from FramelessScrollView), all input events are delivered to widget's event handlers which don't set UserGestureIndicator to DefinitelyUserGesture.

But in safari, all input events are delivered by webview and handled by event handlers in WebCore/page/EventHandler.cpp) in which UserGestureIndicator is set to DefinitelyUserGesture.

I also changed the V8 version ScriptController::processingUserGesture to match the JSC version as much as possible.

[Johnny(Jianning) Ding]

Created attachment 59650 [details]
patch v2

[Adam Barth]

Comment on attachment 59650 [details]
patch v2

Much better.  Thanks.

[Johnny(Jianning) Ding]

Since it is not actual v8 binding bug, I removed the [V8 binding] from the summary

[Johnny(Jianning) Ding]

Hi Adam,

Sorry to bother you again,

I think I didn't exactly make the V8 version ScriptController::processingUserGesture to match the JSC version since I missed one thing in patch v2,

In JSC version ScriptController::processingUserGesture, if there is no available event, then use the returned value of UserGestureIndicator::processingUserGesture() to indicate the user gesture status.

In V8, we should match this. Please see my patch v3.

Sorry again for the inconvenience.

[Johnny(Jianning) Ding]

Created attachment 59755 [details]
patch v3

[WebKit Commit Bot]

Comment on attachment 59650 [details]
patch v2

Rejecting patch 59650 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
e
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19223 test cases.
fast/workers/storage/execute-sql-args-worker.html -> crashed

Exiting early after 1 failures. 15317 tests run.
633.17s total testing time

15316 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3341799

[Adam Barth]

Comment on attachment 59755 [details]
patch v3

ok

[WebKit Commit Bot]

Comment on attachment 59755 [details]
patch v3

Clearing flags on attachment: 59755

Committed r61941: <http://trac.webkit.org/changeset/61941>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61941 might have broken GTK Linux 64-bit Release

[Adam Barth]

Strange.  Does GTK use V8?  Oh, I bet they just don't implemented the needed layoutTestController methods.

--- /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/fast/events/popup-when-select-change-expected.txt	2010-06-25 22:51:01.000000000 -0700
+++ /var/lib/buildbot/build/gtk-linux-64-release/build/layout-test-results/fast/events/popup-when-select-change-actual.txt	2010-06-25 22:51:00.000000000 -0700
@@ -1,4 +1,5 @@
-ALERT: PASSED
+CONSOLE MESSAGE: line 19: TypeError: Result of expression 'eventSender.keyDown' [null] is not an object.
+FAIL: Timed out waiting for notifyDone to be called
  If the pop-up was not blocked then there will be an PASS message. Otherwise, the test fails.

[Johnny(Jianning) Ding]

Adam,

Seems like there are some limitations when writing event related testcases on Linux.
I changed the test case to make it work on both Mac & Linux by referring to other testcases. Please review the patch V4.

Thanks!

[Johnny(Jianning) Ding]

Created attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

[Adam Barth]

Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Huh.  Interesting.  I wonder why Linux behaves different...  Maybe there's some problem with event sender?

[Johnny(Jianning) Ding]

Hi Adam

For error: 'eventSender.keyDown' [null] is not an object, I think it's a problem of event sender on linux. But have no time to dig in now, will take a look later.

Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch?

Thanks!

[Adam Barth]

> For error: 'eventSender.keyDown' [null] is not an object, I think it's a problem of event sender on linux. But have no time to dig in now, will take a look later.

Yes.  That function of layoutTestController isn't implemented on Gtk, which means they can't run a ton of tests.

[Adam Barth]

> Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch?

Is the test on the appropriate Skip lists?  If so, we can land it using commit-queue.  However, if it's going to make some builders red, we can't land it.

[Johnny(Jianning) Ding]

(In reply to comment #20)
> > Since the test is broken on linux, would you or someone who has commit privilege can help me land this patch?
> 
> Is the test on the appropriate Skip lists?  If so, we can land it using commit-queue.  However, if it's going to make some builders red, we can't land it.

Yes, this test is in appropriate Skip lists, they are LayoutTests/platform/chromium/test_expectations.txt (Chromium:Linux) and LayoutTests/platform/gtk/Skipped.

Since it is only affected Linux version, I have tested this test case on my local Linux build of both Chrome and WebKit, they are all green.
So I think it's safe to land the patch 4.

[Adam Barth]

Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

okiedokes

[WebKit Commit Bot]

Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Rejecting patch 59891 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
iling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19232 test cases.
fast/dom/HTMLScriptElement/script-set-src.html -> failed

Exiting early after 1 failures. 6663 tests run.
147.91s total testing time

6662 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3356037

[Johnny(Jianning) Ding]

(In reply to comment #23)
> (From update of attachment 59891 [details])
> Rejecting patch 59891 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
> Last 500 characters of output:
> iling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
> Testing 19232 test cases.
> fast/dom/HTMLScriptElement/script-set-src.html -> failed
> 
> Exiting early after 1 failures. 6663 tests run.
> 147.91s total testing time
> 
> 6662 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 1 test case (<1%) had stderr output
> 
> Full output: http://webkit-commit-queue.appspot.com/results/3356037

Interesting, since the patch V4 only affect a test file (fast/events/popup-when-select-change.html). I am sure that it doesn't affect  script-set-src.html. 
Anything should I do?

[Eric Seidel (no email)]

Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Possible it was a flaky test.

[WebKit Commit Bot]

Comment on attachment 59891 [details]
patch V4 to make the test work on both Mac & Linux.

Clearing flags on attachment: 59891

Committed r62088: <http://trac.webkit.org/changeset/62088>

[Johnny(Jianning) Ding]

(In reply to comment #25)
> (From update of attachment 59891 [details])
> Possible it was a flaky test.

Thanks Eric!

[Johnny(Jianning) Ding]

Make this bug as fixed