Summary: | Use of window.open & window.close can cause crash | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Olivier Le Floch <alakazam> | ||||||||
Component: | DOM | Assignee: | 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
Olivier Le Floch
2006-04-08 18:12:16 PDT
Created attachment 7594 [details]
crash log for ToT
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. 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> 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 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 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.
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 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)
(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. (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! 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?) (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. WebKit Radar is 4491703 |