Bug 8272

Summary: Use of window.open & window.close can cause crash
Product: WebKit Reporter: Olivier Le Floch <alakazam>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alice.barraclough, ap, darin, ian, joost
Priority: P1 Keywords: InRadar
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
crash log for ToT
none
proposed fix
darin: review-
revised fix darin: review+

Olivier Le Floch
Reported 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 } ?> //////////
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-
DetailsFormatted DiffDiff
revised fix (7.73 KB, patch)
2006-07-19 11:01 PDT, Alexey Proskuryakov 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2006-04-09 01:20:11 PDT
Created attachment 7594 [details] crash log for ToT
Alexey Proskuryakov
Comment 2 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.
Alice Liu
Comment 3 2006-05-21 22:38:02 PDT
<rdar://problem/4491703>
David Carson
Comment 4 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>
Alexey Proskuryakov
Comment 5 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.
Darin Adler
Comment 6 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.
Darin Adler
Comment 7 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.
Alexey Proskuryakov
Comment 8 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.
Darin Adler
Comment 9 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)
Alexey Proskuryakov
Comment 10 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.
Darin Adler
Comment 11 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!
Alexey Proskuryakov
Comment 12 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?)
Darin Adler
Comment 13 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.
David Harrison
Comment 14 2006-07-21 10:06:15 PDT
WebKit Radar is 4491703
Note You need to log in before you can comment on or make changes to this bug.