Bug 3784 – JavaScript save dialog disappears right away (sheet triggers blur event)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

If a javascript-generated page in a new window contains code to close this window as soon as any click 
happens, and the user has a save dialog box open at this moment, Safari crashes immediately.

Example:
1. Go to http://dfdl.de/galerie_content/DFDL.php?folge=44
2. Click on the first picture
3. Right-click on the picture on the newly opened page, press and hold option and select "Save Image 
As..." -> Crash happens.

This is a reproducible crash, so I have set it to P1.

[Oliver Hunt]

I can't reproduce -- although the save as dialog disappears immediately...

Alright, this is strange, as I've created a new user on the same machine and it doesn't crash with this new 
user. Maybe some bad behaving extension? I've tried to pinpoint a bad extension amongst the dozens I've 
got, adding extension after extension to the new user without being able to crash it once. After 2 hours I 
gave up.

Any hints on how to isolate this bug, short of copying all my user files to a new user, then deleting stuff 
from it until it works again? Cause I can't do that, not enough hd space :-( Would a crash report help in 
this occasion?

[Oliver Hunt]

> from it until it works again? Cause I can't do that, not enough hd space :-( Would a crash report help in 
> this occasion?

Yes, even if all it does is clear webkit of responsibility :)

[David Storey]

(In reply to comment #1)
> I can't reproduce -- although the save as dialog disappears immediately...

I can't reproduce either, and get the same problem with the save dialogue.  Maybe that is a bug in itself, 
as I've not noticed this before and it should really let you choose a location when you ctrl-option click.  

[Joost de Valk (AlthA)]

K, i get the same problem with the save dialog, renaming the bug to reflect that, and asking the assignee 
of this bug to check for crashes too, since we can't reproduce them but they probably have happened. 
Confirming the bug, newly named: Javascript save dialog dissapearing instantly.

[Darin Adler]

I can't reproduce the bug described here.

[Darin Adler]

Reducing priority since this is not a reproducible crash.

[Alexey Proskuryakov]

Created attachment 7022 [details]
crash log

[Alexey Proskuryakov]

I can reproduce the crash with the following steps:

0. Enter the following command in Terminal:
mv ~/Library/Preferences/com.apple.Safari.plist ~/Library/Preferences/com.apple.Safari.plist.backup
defaults write com.apple.Safari NSNavPanelExpandedStateForSaveMode -bool YES
run-safari
1. Go to http://dfdl.de/galerie_content/DFDL.php?folge=44
2. Click on the first picture
3. Right-click on the picture on the newly opened page, press and hold option
and select "Save Image As..." -> Crash happens.

Whether this is a bug in WebKit, Safari or AppKit is a different question, of course.

[Darin Adler]

I still can't reproduce a crash. Alexey's steps do reliably reproduce a case where the window is closed right away.

The reason for that is that the site has a blur handler that closes the window. The bug here is that we deliver a blur event when the sheet comes up, which isn't right. The sheet shouldn't count for purposes of the JavaScript blur and focus events.

The source of that trouble is -[WebHTMLView _updateFocusState]. It needs to treat the window as key for purposes of focus and blur events even when there's a sheet up. So instead of just calling -[NSWindow isKeyWindow] it should probably call -[NSApplication keyWindow] and check if the key window is either the WebHTMLView's own window or the attachedSheet of that window. Similarly, if the key window is a modal dialog I don't think we should deliver a blur event.

Two things that might make this tricky to fix:

    1) We probably want to treat the window as not focused for purposes of the window's appearance. So we should be careful to only do the new check for the value for setWindowHasFocus: and leave setDisplaysWithFocusAttributes: alone, still using isKeyWindow.

    2) -[WebHTMLView _updateFocusState] is currently called from -[WebHTMLView windowDidBecomeKey:] and -[WebHTMLView windowDidResignKey:], and we register for those only for the window itself in -[WebHTMLView addWindowObservers]. If we leave the window in a focused state when the sheet becomes the key window, we won't get any notification at all when the sheet resigns key if you click on some other window. And we'd want to send a blur event in that case. One way to fix this would be to register for those notifications on a sheet when the sheet is up. We could register for NSWindowWillBeginSheetNotification and NSWindowDidEndSheetNotification notifications, and then set up and tear down an observer for the sheet. A simpler way would be to register for NSWindowDidBecomeKeyNotification and NSWindowDidResignKeyNotification globally without passing a window pointer, and then check the window pointer in the handler method.

None of this necessarily has to do with the crash; I can't reproduce the crash so I'm not sure. You could probably still reproduce the crash by just having JavaScript code in some other window that closes this window. It doesn't have to be from the blur event. Or you could still trigger the blur event with the save dialog up by clicking in another window.

[Darin Adler]

Created attachment 7058 [details]
patch to avoid giving focus/blur events when sheets appear/disappear

Here's a patch implementing my suggestion. Since I can't reproduce the crash, I don't know if this fixes the crash, or perhaps just makes it harder to reproduce (since you can still make the sheet go away if you click in another window).

[Alexey Proskuryakov]

I could not reproduce the crash with this patch applied, even when dismissing the popup by clicking in other windows.

With 2.0.3 and ToT, it was reproducible on both machines I tried, G4/1.25DP and G4/867DP (even with one processor disabled).

[Darin Adler]

Created attachment 7177 [details]
better version of focus/blur patch

[Alice Liu]

<rdar://problem/4483827>

[Maciej Stachowiak]

Comment on attachment 7177 [details]
better version of focus/blur patch

r=me