Bug 96480

Summary:

REGRESSION(r128270): It made fast/events/popup-blocking-timers.html flakey

Product:

WebKit

Reporter:

Csaba Osztrogonác <ossy>

Component:

New Bugs

Assignee:

jochen

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dbates, gyuyoung.kim, jochen, ossy, rakuco, webkit.review.bot, zan

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

79666, 96475

Attachments:

Description	Flags
Patch	none
Patch for landing	none

Csaba Osztrogonác

Reported 2012-09-12 01:45:34 PDT

r128270 -------- --- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-expected.txt +++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-actual.txt @@ -1,16 +1,2 @@ -Click Here (6 times) -Test calling window.open() directly. A popup should be allowed. -PASS newWindow is non-null. -Test calling window.open() with a 0 ms delay. A popup should be allowed. -PASS newWindow is non-null. -Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval. -PASS newWindow is non-null. -Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval. -PASS newWindow is undefined. -Test calling window.open() in a nested call to setTimeout(). A popup should not be allowed. -PASS newWindow is undefined. -Test calling window.open() with a 1000 ms delay. A popup should be allowed. -PASS newWindow is non-null. -Test calling window.open() with a 1001 ms delay. A popup should not be allowed. -PASS newWindow is undefined. +FAIL: Timed out waiting for notifyDone to be called r128275 -------- --- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-expected.txt +++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/events/popup-blocking-timers-actual.txt @@ -5,12 +5,12 @@ PASS newWindow is non-null. Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval. PASS newWindow is non-null. +Test calling window.open() with a 1000 ms delay. A popup should be allowed. +PASS newWindow is non-null. +Test calling window.open() in a nested call to setTimeout(). A popup should not be allowed. +PASS newWindow is undefined. Test calling window.open() in a 100 ms interval. A popup should only be allowed on the first execution of the interval. PASS newWindow is undefined. -Test calling window.open() in a nested call to setTimeout(). A popup should not be allowed. -PASS newWindow is undefined. -Test calling window.open() with a 1000 ms delay. A popup should be allowed. -PASS newWindow is non-null. Test calling window.open() with a 1001 ms delay. A popup should not be allowed. PASS newWindow is undefined.

Attachments
Patch (24.17 KB, patch) 2012-10-02 07:12 PDT, jochen	no flags	Details Formatted Diff Diff
Patch for landing (24.28 KB, patch) 2012-10-02 11:50 PDT, jochen	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Csaba Osztrogonác

Comment 1 2012-09-12 03:41:18 PDT

I skipped it - https://trac.webkit.org/changeset/128288 Please unskip it with the proper fix.

jochen

Comment 2 2012-09-17 04:42:21 PDT

(In reply to comment #1) > I skipped it - https://trac.webkit.org/changeset/128288 > Please unskip it with the proper fix. I checked the chromium test logs, and the test used to be flaky before (esp. this case when the ordering of events changes). I could imagine that it is less flaky, if we split it up in six individual tests, wdyt?

Adam Barth

Comment 3 2012-09-17 11:23:44 PDT

Sure, we can give that a try.

jochen

Comment 4 2012-10-02 07:09:46 PDT

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

jochen

Comment 5 2012-10-02 07:12:05 PDT

Created attachment 166686 [details] Patch

Daniel Bates

Comment 6 2012-10-02 09:16:13 PDT

Comment on attachment 166686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166686&action=review Looks sane to me. > LayoutTests/fast/events/popup-blocking-timers1.html:31 > + function clickButton() { > + var button = document.getElementById("test"); > + var buttonX = button.offsetLeft + button.offsetWidth / 2; > + var buttonY = button.offsetTop + button.offsetHeight / 2; > + if (window.eventSender) { > + eventSender.mouseMoveTo(buttonX, buttonY); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > + } I know that you derived this function from the function of the same name in the original test. I take it that it's necessary to use EventSender to actually simulate a mouse click of a person as opposed to calling click() on the HTML button element? > LayoutTests/fast/events/popup-blocking-timers2.html:1 > +<head> Unless you intend to test quirks mode, I suggest that we include a DOCTYPE, say <!DOCTYPE html>. > LayoutTests/fast/events/popup-blocking-timers2.html:33 > + function clickButton() { > + var button = document.getElementById("test"); > + var buttonX = button.offsetLeft + button.offsetWidth / 2; > + var buttonY = button.offsetTop + button.offsetHeight / 2; > + if (window.eventSender) { > + eventSender.mouseMoveTo(buttonX, buttonY); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > + } Ditto. > LayoutTests/fast/events/popup-blocking-timers3.html:41 > + function clickButton() { > + var button = document.getElementById("test"); > + var buttonX = button.offsetLeft + button.offsetWidth / 2; > + var buttonY = button.offsetTop + button.offsetHeight / 2; > + if (window.eventSender) { > + eventSender.mouseMoveTo(buttonX, buttonY); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > + } Ditto. > LayoutTests/fast/events/popup-blocking-timers4.html:35 > + function clickButton() { > + var button = document.getElementById("test"); > + var buttonX = button.offsetLeft + button.offsetWidth / 2; > + var buttonY = button.offsetTop + button.offsetHeight / 2; > + if (window.eventSender) { > + eventSender.mouseMoveTo(buttonX, buttonY); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > + } Ditto. > LayoutTests/fast/events/popup-blocking-timers5.html:36 > + function clickButton() { > + var button = document.getElementById("test"); > + var buttonX = button.offsetLeft + button.offsetWidth / 2; > + var buttonY = button.offsetTop + button.offsetHeight / 2; > + if (window.eventSender) { > + eventSender.mouseMoveTo(buttonX, buttonY); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > + } Ditto. > LayoutTests/fast/events/popup-blocking-timers6.html:36 > + function clickButton() { > + var button = document.getElementById("test"); > + var buttonX = button.offsetLeft + button.offsetWidth / 2; > + var buttonY = button.offsetTop + button.offsetHeight / 2; > + if (window.eventSender) { > + eventSender.mouseMoveTo(buttonX, buttonY); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > + } Ditto.

Daniel Bates

Comment 7 2012-10-02 09:18:51 PDT

(In reply to comment #6) > > LayoutTests/fast/events/popup-blocking-timers2.html:1 > > +<head> > > Unless you intend to test quirks mode, I suggest that we include a DOCTYPE, say <!DOCTYPE html>. Feel free to disregard this comment as the original test omitted a <!DOCTYPE> declaration and I don't anticipate that the result of this test will change in quirks mode.

jochen

Comment 8 2012-10-02 11:50:43 PDT

Created attachment 166724 [details] Patch for landing

jochen

Comment 9 2012-10-02 11:51:39 PDT

(In reply to comment #6) > (From update of attachment 166686 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166686&action=review > > Looks sane to me. > > > LayoutTests/fast/events/popup-blocking-timers1.html:31 > > + function clickButton() { > > + var button = document.getElementById("test"); > > + var buttonX = button.offsetLeft + button.offsetWidth / 2; > > + var buttonY = button.offsetTop + button.offsetHeight / 2; > > + if (window.eventSender) { > > + eventSender.mouseMoveTo(buttonX, buttonY); > > + eventSender.mouseDown(); > > + eventSender.mouseUp(); > > + } > > + } > > I know that you derived this function from the function of the same name in the original test. I take it that it's necessary to use EventSender to actually simulate a mouse click of a person as opposed to calling click() on the HTML button element? The call to click() would not be counted as a user action, and therefore no popups would be allowed at all > > > LayoutTests/fast/events/popup-blocking-timers2.html:1 > > +<head> > > Unless you intend to test quirks mode, I suggest that we include a DOCTYPE, say <!DOCTYPE html>. done

WebKit Review Bot

Comment 10 2012-10-02 13:10:59 PDT

Comment on attachment 166724 [details] Patch for landing Clearing flags on attachment: 166724 Committed r130200: <http://trac.webkit.org/changeset/130200>

WebKit Review Bot

Comment 11 2012-10-02 13:11:04 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.