Created attachment 33141 [details] onbeforeonload is not called because child frame is focused. The bug I have is about onbeforeonload assigned to a window which contain an iframe or a frameset. The handler is properly called if we back the document or if we load the document and immediately close the window. Yet if the contained frame or iframe is focused (using JS as in the attached reduction) when the window is closed then, the window beforeonload handler is not called. Note (1): FAIL: Safari4/WinddowsXP 4.0.2 (530.19.1) Note (2): OK: Safari4/Mac Note(3): related bugs: https://bugs.webkit.org/show_bug.cgi?id=19418 as well as https://bugs.webkit.org/show_bug.cgi?id=21669 are about an onbeforeunload handler when it is assigned to the frame content or the iframe itself. In the bug described above, we do not assign to iframe or frame but on the parent window. Note (2): Safari4/Mac is ok.
Created attachment 33142 [details] onbeforeonload is not called because child iframe is focused.
*** Bug 28206 has been marked as a duplicate of this bug. ***
Created attachment 48156 [details] Patch
(In reply to comment #3) > Created an attachment (id=48156) [details] > Patch This bug affects both WebKit on Windows and Chromium. See: http://code.google.com/p/chromium/issues/detail?id=32615 http://code.google.com/p/chromium/issues/detail?id=17157 I wrote a layout test for it, but it doesn't seem to be able to reproduce the bug. The bug only occurs if you close the browser window, not if you call window.close() from JavaScript. I do have a Chromium UI test ready to commit that shows the bug is fixed on that side.
Comment on attachment 48156 [details] Patch This looks like the correct fix. It is good to make Windows and Chromium match the Mac in this respect. > + Layout tests aren't able to test this bug, since it requires closing > + the actual browser window, not calling window.close(). Did you try to make a test? I understand that window.close() is not helpful to test this, but it should be testable in DumpRenderTree. We have existing tests for beforeunload and they had no problem with this. For example, onunload-clears-onbeforeunload.html. If you do need a way to test this, we would typically add a way to do it to DumpRenderTree. We have features already to test opening and closing multiple windows. You can find 73 tests that use the layoutTestController.setCanOpenWindows() and you can probably create a test that way. If you try all these things and fail, at least you should add a test to the WebCore/manual-tests directory. review- because this does need a test, and further, an automated test should be possible
Created attachment 48247 [details] Attempted layout test I did try to write a layout test for this (based on the other unload tests) before uploading the patch, and I've attached it here. It does use setCanOpenWindows and setCloseRemaningWindowsWhenComplete. I was never able to get the test to reproduce the bug, though. Calling window.close() or navigating won't trigger the bug-- the window has to be closed via the WebView. The closest thing I saw was layoutTestController.setCallCloseOnWebViews(), but I didn't see a way to close the window with that during the test, at a point when the resulting alert would still show up in the output. I could adapt this test to go into WebCore/manual-tests, or I could try to add a way to directly close a particular WebView to layoutTestController. Not sure if that's useful in other tests, though. Thoughts?
Created attachment 48276 [details] Patch
(In reply to comment #7) > Created an attachment (id=48276) [details] > Patch Here's a version with a manual test, since I haven't been able to figure out how to get a layout test to call WebView::shouldClose. If there is a way, I'm happy to update the layout test draft that I attached in comment #6 and include that instead.
You can always add an API to DumpRenderTree to call WebView::shouldClose. :)
Comment on attachment 48276 [details] Patch An automated test would be much better. I'm sure we'll want to do lots of other tests relating to closing windows. r=me but please don't drop the effort to make an automated test
Comment on attachment 48276 [details] Patch Clearing flags on attachment: 48276 Committed r54519: <http://trac.webkit.org/changeset/54519>
All reviewed patches have been landed. Closing bug.
Thanks-- I'll take a look at extending DumpRenderTree to support an automated test for this.
Thanks.
Created attachment 48541 [details] Patch
(In reply to comment #15) > Created an attachment (id=48541) [details] > Patch Sorry for the delay. Here's another patch to add an automated layout test for this bug, by adding a method to LayoutTestController. Let me know if I missed a place I need to update, since I haven't added things there before.
Attachment 48541 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/256764
Created attachment 48574 [details] Patch
Comment on attachment 48574 [details] Patch LGTM. Manual tests -> automatic tests = win.
Re-open so the commit-bot will see the new patch.
Comment on attachment 48574 [details] Patch Rejecting patch 48574 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adam Barth', '--force']" exit_code: 1 Last 500 characters of output: cused-iframe-expected.txt patching file LayoutTests/fast/events/onbeforeunload-focused-iframe.html patching file LayoutTests/fast/events/resources/onbeforeunload-focused-iframe-frame.html patching file LayoutTests/platform/gtk/Skipped Hunk #1 FAILED at 5814. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej patching file LayoutTests/platform/qt/Skipped Hunk #1 FAILED at 5077. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej Full output: http://webkit-commit-queue.appspot.com/results/270611
Created attachment 48906 [details] Patch
(In reply to comment #21) > (From update of attachment 48574 [details]) > Rejecting patch 48574 from commit-queue. > Looks like that was just an svn conflict on the Skipped files. I resolved it and uploaded a new patch.
Comment on attachment 48906 [details] Patch Rejecting patch 48906 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12190 test cases. fast/events/onchange-click-hang.html -> failed Exiting early after 1 failures. 6339 tests run. 94.80s total testing time 6338 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 2 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/277248
Comment on attachment 48906 [details] Patch Flaky test?
Comment on attachment 48906 [details] Patch Rejecting patch 48906 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1 Last 500 characters of output: itTools/DumpRenderTree/qt/LayoutTestControllerQt.h.rej patching file WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp patching file LayoutTests/ChangeLog patching file LayoutTests/fast/events/onbeforeunload-focused-iframe-expected.txt patching file LayoutTests/fast/events/onbeforeunload-focused-iframe.html patching file LayoutTests/fast/events/resources/onbeforeunload-focused-iframe-frame.html patching file LayoutTests/platform/gtk/Skipped patching file LayoutTests/platform/qt/Skipped Full output: http://webkit-commit-queue.appspot.com/results/276274
Created attachment 48919 [details] Patch
(In reply to comment #27) > Created an attachment (id=48919) [details] > Patch Fixes the svn conflict in LayoutTestControllerQt.{h,cpp}.
Comment on attachment 48919 [details] Patch Rejecting patch 48919 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12190 test cases. fast/events/onchange-click-hang.html -> failed Exiting early after 1 failures. 6339 tests run. 95.43s total testing time 6338 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 2 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/277303
*** Bug 35046 has been marked as a duplicate of this bug. ***
I'll look at the commit-bot machine, but it looks like there is an issue with this patch.
I saw nothing abnormal with the commit-bot machine.
Comment on attachment 48919 [details] Patch This fails on my box too. It looks like the new test is leaking state into another test. Did you run-webkit-tests before uploading this patch? --- /tmp/layout-test-results/fast/events/onchange-click-hang-expected.txt 2010-02-18 11:21:04.000000000 -0800 +++ /tmp/layout-test-results/fast/events/onchange-click-hang-actual.txt 2010-02-18 11:21:04.000000000 -0800 @@ -1,3 +1,4 @@ +ALERT: beforeUnload These checkboxes call their own and each others click() method from their onchange callbacks. Click the first checkbox. This should not crash or hang. This also shows that click() is protected on per-element basis. checkbox1 onchange enter
(In reply to comment #33) > (From update of attachment 48919 [details]) > This fails on my box too. It looks like the new test is leaking state into > another test. Did you run-webkit-tests before uploading this patch? > > --- /tmp/layout-test-results/fast/events/onchange-click-hang-expected.txt > 2010-02-18 11:21:04.000000000 -0800 > +++ /tmp/layout-test-results/fast/events/onchange-click-hang-actual.txt > 2010-02-18 11:21:04.000000000 -0800 > @@ -1,3 +1,4 @@ > +ALERT: beforeUnload > These checkboxes call their own and each others click() method from their > onchange callbacks. Click the first checkbox. This should not crash or hang. > This also shows that click() is protected on per-element basis. > > checkbox1 onchange enter Ack, interactions across tests... I'll take a look.
Created attachment 49040 [details] Patch
(In reply to comment #35) > Created an attachment (id=49040) [details] > Patch This patch clears the beforeunload handler once the test finishes, since it appeared to still be present in the subsequent test. I ran the full set of layout tests, and this resolves the onchange-click-hang.html failure (since that happened to be the next test to run after this one). There were a handful of other failures, but they look like they had more to do with my machine than the patch (e.g., no WebGL context found, etc).
Attachment 49040 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/283739
Why would we need to clear the onbeforeunload handler in javascript? Shouldn't navigating the frame clear the window object and any associated listeners?
(In reply to comment #38) > Why would we need to clear the onbeforeunload handler in javascript? Shouldn't > navigating the frame clear the window object and any associated listeners? That's a very good question, but I can only think that it has something to do with the way the tests are run. Two things that stand out to me: (1) Tests that set an unload listener end up having a "main frame - has 1 onunload handler(s)" line printed in their output, suggesting that DRT knows about these handlers. (2) The other beforeunload tests I've found either clear the handler (e.g., fast/events/onunload-clears-onbeforeunload.html) or do nothing in it (eg., dom/Window/HTMLBodyElement-window-event-Listener-attributes.html). I'm not familiar with how DRT deals with those event listeners, though. BTW, I have a fix coming for the failed compile on the last patch. It was just a failed SVN merge on LayoutTestController.cpp, but I'm double checking that the whole thing compiles and tests before uploading again.
Sam may have some clue why the onload listener doesn't appear to be getting cleared. Or it might just be that the logging is happending between tests and bleeding into the next test somehow.
Created attachment 49047 [details] Patch
Comment on attachment 49047 [details] Patch Sorry we dropped the ball on this patch Charlie. Thanks for your persistence.
Comment on attachment 49047 [details] Patch Rejecting patch 49047 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adam Barth', '--force']" exit_code: 1 Last 500 characters of output: ching file LayoutTests/fast/events/onbeforeunload-focused-iframe-expected.txt patching file LayoutTests/fast/events/onbeforeunload-focused-iframe.html patching file LayoutTests/fast/events/resources/onbeforeunload-focused-iframe-frame.html patching file LayoutTests/platform/gtk/Skipped Hunk #1 succeeded at 5762 with fuzz 2 (offset -92 lines). patching file LayoutTests/platform/qt/Skipped Hunk #1 FAILED at 5068. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej Full output: http://webkit-commit-queue.appspot.com/results/1017131
Charlie isn't a committer. So it seems that the cq- is effectively the same as an r-. Please revise and resubmit this patch. (Unless Adam would like to land it?)
(In reply to comment #44) > Charlie isn't a committer. So it seems that the cq- is effectively the same as > an r-. Please revise and resubmit this patch. (Unless Adam would like to land > it?) Adam and I have talked about coordinating on Friday to get a fresh patch uploaded and committed, since these files seem to pick up merge conflicts pretty quickly if you let them sit.
Committed r56606
(In reply to comment #46) > Committed r56606 Thanks Adam!