Bug 27481 - onbeforeunload not called at window close + frame or iframe focused
Summary: onbeforeunload not called at window close + frame or iframe focused
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords: PlatformOnly
: 28206 35046 (view as bug list)
Depends on: 35046
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-20 21:02 PDT by Marc Bonnier
Modified: 2010-03-26 17:14 PDT (History)
13 users (show)

See Also:


Attachments
onbeforeonload is not called because child frame is focused. (441 bytes, text/html)
2009-07-20 21:02 PDT, Marc Bonnier
no flags Details
onbeforeonload is not called because child iframe is focused. (405 bytes, text/html)
2009-07-20 21:02 PDT, Marc Bonnier
no flags Details
Patch (2.91 KB, patch)
2010-02-04 10:47 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Attempted layout test (4.43 KB, text/plain)
2010-02-05 12:22 PST, Charles Reis
no flags Details
Patch (6.07 KB, patch)
2010-02-05 17:43 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (13.66 KB, patch)
2010-02-10 19:06 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (14.15 KB, patch)
2010-02-11 09:33 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (14.51 KB, patch)
2010-02-17 10:20 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (14.45 KB, patch)
2010-02-17 11:56 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2010-02-18 13:42 PST, Charles Reis
no flags Details | Formatted Diff | Diff
Patch (14.65 KB, patch)
2010-02-18 15:51 PST, Charles Reis
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Bonnier 2009-07-20 21:02:00 PDT
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.
Comment 1 Marc Bonnier 2009-07-20 21:02:55 PDT
Created attachment 33142 [details]
onbeforeonload is not called because child iframe is focused.
Comment 2 Alexey Proskuryakov 2009-08-11 22:16:09 PDT
*** Bug 28206 has been marked as a duplicate of this bug. ***
Comment 3 Charles Reis 2010-02-04 10:47:42 PST
Created attachment 48156 [details]
Patch
Comment 4 Charles Reis 2010-02-04 10:50:50 PST
(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 5 Darin Adler 2010-02-05 11:37:44 PST
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
Comment 6 Charles Reis 2010-02-05 12:22:49 PST
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?
Comment 7 Charles Reis 2010-02-05 17:43:41 PST
Created attachment 48276 [details]
Patch
Comment 8 Charles Reis 2010-02-05 17:48:15 PST
(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.
Comment 9 Adam Barth 2010-02-05 18:20:50 PST
You can always add an API to DumpRenderTree to call WebView::shouldClose.  :)
Comment 10 Darin Adler 2010-02-07 13:11:13 PST
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 11 WebKit Commit Bot 2010-02-08 16:32:32 PST
Comment on attachment 48276 [details]
Patch

Clearing flags on attachment: 48276

Committed r54519: <http://trac.webkit.org/changeset/54519>
Comment 12 WebKit Commit Bot 2010-02-08 16:32:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Charles Reis 2010-02-08 17:07:07 PST
Thanks-- I'll take a look at extending DumpRenderTree to support an automated test for this.
Comment 14 Marc Bonnier 2010-02-08 23:12:45 PST
Thanks.
Comment 15 Charles Reis 2010-02-10 19:06:53 PST
Created attachment 48541 [details]
Patch
Comment 16 Charles Reis 2010-02-10 19:09:14 PST
(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.
Comment 17 WebKit Review Bot 2010-02-10 21:07:25 PST
Attachment 48541 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/256764
Comment 18 Charles Reis 2010-02-11 09:33:12 PST
Created attachment 48574 [details]
Patch
Comment 19 Adam Barth 2010-02-16 12:52:52 PST
Comment on attachment 48574 [details]
Patch

LGTM.  Manual tests -> automatic tests = win.
Comment 20 Adam Barth 2010-02-16 12:53:22 PST
Re-open so the commit-bot will see the new patch.
Comment 21 WebKit Commit Bot 2010-02-16 17:08:03 PST
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
Comment 22 Charles Reis 2010-02-17 10:20:40 PST
Created attachment 48906 [details]
Patch
Comment 23 Charles Reis 2010-02-17 10:22:00 PST
(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 24 WebKit Commit Bot 2010-02-17 10:48:57 PST
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 25 Adam Barth 2010-02-17 11:30:06 PST
Comment on attachment 48906 [details]
Patch

Flaky test?
Comment 26 WebKit Commit Bot 2010-02-17 11:41:36 PST
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
Comment 27 Charles Reis 2010-02-17 11:56:23 PST
Created attachment 48919 [details]
Patch
Comment 28 Charles Reis 2010-02-17 11:57:25 PST
(In reply to comment #27)
> Created an attachment (id=48919) [details]
> Patch

Fixes the svn conflict in LayoutTestControllerQt.{h,cpp}.
Comment 29 WebKit Commit Bot 2010-02-17 12:19:43 PST
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
Comment 30 Eric Seidel (no email) 2010-02-17 12:22:29 PST
*** Bug 35046 has been marked as a duplicate of this bug. ***
Comment 31 Eric Seidel (no email) 2010-02-17 12:22:53 PST
I'll look at the commit-bot machine, but it looks like there is an issue with this patch.
Comment 32 Eric Seidel (no email) 2010-02-17 12:37:31 PST
I saw nothing abnormal with the commit-bot machine.
Comment 33 Adam Barth 2010-02-18 11:31:59 PST
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
Comment 34 Charles Reis 2010-02-18 11:45:43 PST
(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.
Comment 35 Charles Reis 2010-02-18 13:42:28 PST
Created attachment 49040 [details]
Patch
Comment 36 Charles Reis 2010-02-18 13:46:09 PST
(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).
Comment 37 WebKit Review Bot 2010-02-18 13:46:54 PST
Attachment 49040 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/283739
Comment 38 Eric Seidel (no email) 2010-02-18 13:51:25 PST
Why would we need to clear the onbeforeunload handler in javascript?  Shouldn't navigating the frame clear the window object and any associated listeners?
Comment 39 Charles Reis 2010-02-18 14:06:37 PST
(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.
Comment 40 Eric Seidel (no email) 2010-02-18 14:12:28 PST
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.
Comment 41 Charles Reis 2010-02-18 15:51:10 PST
Created attachment 49047 [details]
Patch
Comment 42 Adam Barth 2010-03-22 10:22:24 PDT
Comment on attachment 49047 [details]
Patch

Sorry we dropped the ball on this patch Charlie.  Thanks for your persistence.
Comment 43 WebKit Commit Bot 2010-03-22 17:55:18 PDT
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
Comment 44 Eric Seidel (no email) 2010-03-24 20:09:57 PDT
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?)
Comment 45 Charles Reis 2010-03-25 09:32:40 PDT
(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.
Comment 46 Adam Barth 2010-03-25 23:35:26 PDT
Committed r56606
Comment 47 Charles Reis 2010-03-26 09:29:58 PDT
(In reply to comment #46)
> Committed r56606

Thanks Adam!