Bug 8272 - Use of window.open & window.close can cause crash
Summary: Use of window.open & window.close can cause crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2006-04-08 18:12 PDT by Olivier Le Floch
Modified: 2006-07-21 10:06 PDT (History)
5 users (show)

See Also:


Attachments
crash log for ToT (24.86 KB, text/plain)
2006-04-09 01:20 PDT, Alexey Proskuryakov
no flags Details
proposed fix (554 bytes, patch)
2006-07-17 11:11 PDT, Alexey Proskuryakov
darin: review-
Details | Formatted Diff | Diff
revised fix (7.73 KB, patch)
2006-07-19 11:01 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Le Floch 2006-04-08 18:12:16 PDT
The following php code generates html+javascript code which rather reliably crashes webkit, by opening a window which reloads it's parent and closes itself.

The crash is very reproductible, but does not seem to happen in a perfectly reliable way.

The crash does not require that the page reload itself many times, on http://www.battle-arenas.net/, the bug has been perfectly reproductible when
	window.opener.location = window.opener.location;
	window.close();
is called, after having opened a window used to send messages to other players via a button with a onClick call to a javascript function.

//////////
<?php
if (isset($_GET['opened'])) {
?><script type="text/javascript">
	window.opener.location = window.opener.location;
	window.close();
</script><?php
} else {
?><script type="text/javascript">
	window.open('test.php?opened=1', '', '');
</script><?php
}
?>
//////////
Comment 1 Alexey Proskuryakov 2006-04-09 01:20:11 PDT
Created attachment 7594 [details]
crash log for ToT
Comment 2 Alexey Proskuryakov 2006-04-09 01:24:33 PDT
Confirmed with ToT. Had to move the mouse pointer to get a crash.

This would benefit from a reduction that worked from a local file, adding NeedsReduction keyword.
Comment 3 Alice Liu 2006-05-21 22:38:02 PDT
<rdar://problem/4491703>
Comment 4 David Carson 2006-07-16 06:42:12 PDT
Tried to create a reduced test case. Code below. Open and closing the window worked fine. Could not find where the defect occurs in the site referenced.

Main.html:
<html>
<A href="#" onClick="window.open('popup.html', '', ''); return false">open</A>
</html>

popup.html:
<html>
<head>
<script type="text/javascript">
function closew() {
        window.opener.location = window.opener.location;
        window.close();
        }
</script>
</head>
<body>
Close window <a href="javascript:closew()">Close</a>
</body>
</html>
Comment 5 Alexey Proskuryakov 2006-07-17 11:11:19 PDT
Created attachment 9527 [details]
proposed fix

This seems to be a bug in Cocoa: [[NSApp currentEvent] window] is invalid. Filed as <rdar://4634031>.

Looks like m_mouseDown doesn't need to be initialized to the current event here anyway, so I made it initialize with zeros.

I don't see how to make a sensible layout test for this problem.
Comment 6 Darin Adler 2006-07-17 11:41:09 PDT
Comment on attachment 9527 [details]
proposed fix

Looks like a good fix. Please write a change log for it and try to figure out how to make a test case for it.
Comment 7 Darin Adler 2006-07-17 11:44:28 PDT
Comment on attachment 9527 [details]
proposed fix

I think it's lame that the default constructor picks up the current event. We should change the default so it works like this and change the current event constructor to be a named static member function. That would be a better way to fix this.

Between that and the lack of a change log I'm going to review- this, but I agree completely about the approach to a fix.
Comment 8 Alexey Proskuryakov 2006-07-19 11:01:38 PDT
Created attachment 9571 [details]
revised fix

Added a manual test case (tricky to use :( ). Changed the default constructor not to pick the current event.
Comment 9 Darin Adler 2006-07-19 22:50:16 PDT
Comment on attachment 9571 [details]
revised fix

I like this one a lot better.

I prefer a slightly different style for the current event constructor. Something like:

    private:
        enum CurrentEventType { CurrentEvent };
        PlatformMouseEvent(CurrentEventType);

    public:
        static PlatformMouseEvent currentEvent();

and then:

    PlatformMouseEvent event = PlatformMouseEvent::currentEvent();

What you've done is quite similar, and pretty good, but I think mine is slightly better.

r=me (if you decide not to do it my way; seems good to go as-is)
Comment 10 Alexey Proskuryakov 2006-07-20 00:10:02 PDT
(In reply to comment #9)

The reason why I generally use this style is that it is more universal (works for noncopyable classes), and more versatile (easier to create objects in heap). Unless there is some drawback that I am not aware of, I think I'd like to keep my version. But since neither reason matters for PlatformMouseEvent, I don't have particularly strong feelings about that.
Comment 11 Darin Adler 2006-07-20 08:47:39 PDT
(In reply to comment #10)
> The reason why I generally use this style is that it is more universal (works
> for noncopyable classes), and more versatile (easier to create objects in
> heap). Unless there is some drawback that I am not aware of, I think I'd like
> to keep my version. But since neither reason matters for PlatformMouseEvent, I
> don't have particularly strong feelings about that.

The enum aspect of my version, at least, is more efficient, and avoids the need to access a global variable. But that's nit picking. What we need now is to get this change landed!
Comment 12 Alexey Proskuryakov 2006-07-20 12:20:29 PDT
Committed revision 15544.

(In reply to comment #11)
Understood! This variation of the trick is new to me, and it definitely looks nicer - I'm only a bit unsure if it's equally safe (can the enum parameter be mixed with an integral one under any circumstances?)
Comment 13 Darin Adler 2006-07-20 13:00:29 PDT
(In reply to comment #12)
> Understood! This variation of the trick is new to me, and it definitely looks
> nicer - I'm only a bit unsure if it's equally safe (can the enum parameter be
> mixed with an integral one under any circumstances?)

I don't think so, but I'm not absolutely sure.

That's less of an issue when the constructor is private, of course.
Comment 14 David Harrison 2006-07-21 10:06:15 PDT
WebKit Radar is 4491703