Bug 27481 – onbeforeunload not called at window close + frame or iframe focused

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Marc Bonnier]

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.

[Marc Bonnier]

Created attachment 33142 [details]
onbeforeonload is not called because child iframe is focused.

[Alexey Proskuryakov]

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

[Charles Reis]

Created attachment 48156 [details]
Patch

[Charles Reis]

(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.

[Darin Adler]

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

[Charles Reis]

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?

[Charles Reis]

Created attachment 48276 [details]
Patch

[Charles Reis]

(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.

[Adam Barth]

You can always add an API to DumpRenderTree to call WebView::shouldClose.  :)

[Darin Adler]

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

[WebKit Commit Bot]

Comment on attachment 48276 [details]
Patch

Clearing flags on attachment: 48276

Committed r54519: <http://trac.webkit.org/changeset/54519>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Charles Reis]

Thanks-- I'll take a look at extending DumpRenderTree to support an automated test for this.

[Marc Bonnier]

Thanks.

[Charles Reis]

Created attachment 48541 [details]
Patch

[Charles Reis]

(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.

[WebKit Review Bot]

Attachment 48541 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/256764

[Charles Reis]

Created attachment 48574 [details]
Patch

[Adam Barth]

Comment on attachment 48574 [details]
Patch

LGTM.  Manual tests -> automatic tests = win.

[Adam Barth]

Re-open so the commit-bot will see the new patch.

[WebKit Commit Bot]

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

[Charles Reis]

Created attachment 48906 [details]
Patch

[Charles Reis]

(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.

[WebKit Commit Bot]

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

[Adam Barth]

Comment on attachment 48906 [details]
Patch

Flaky test?

[WebKit Commit Bot]

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

[Charles Reis]

Created attachment 48919 [details]
Patch

[Charles Reis]

(In reply to comment #27)
> Created an attachment (id=48919) [details]
> Patch

Fixes the svn conflict in LayoutTestControllerQt.{h,cpp}.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

I'll look at the commit-bot machine, but it looks like there is an issue with this patch.

[Eric Seidel (no email)]

I saw nothing abnormal with the commit-bot machine.

[Adam Barth]

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

[Charles Reis]

(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.

[Charles Reis]

Created attachment 49040 [details]
Patch

[Charles Reis]

(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).

[WebKit Review Bot]

Attachment 49040 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/283739

[Eric Seidel (no email)]

Why would we need to clear the onbeforeunload handler in javascript?  Shouldn't navigating the frame clear the window object and any associated listeners?

[Charles Reis]

(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.

[Eric Seidel (no email)]

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.

[Charles Reis]

Created attachment 49047 [details]
Patch

[Adam Barth]

Comment on attachment 49047 [details]
Patch

Sorry we dropped the ball on this patch Charlie.  Thanks for your persistence.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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?)

[Charles Reis]

(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.

[Adam Barth]

Committed r56606

[Charles Reis]

(In reply to comment #46)
> Committed r56606

Thanks Adam!