Closed Bug 1260850 Opened 8 years ago Closed 8 years ago

Open File Dialog Box Stuck

Categories

(Firefox :: File Handling, defect, P1)

45 Branch
x86
macOS
defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: dhtmlkitchen, Assigned: spohl)

References

(Blocks 1 open bug)

Details

(Whiteboard: tpi:+)

Attachments

(2 files, 2 obsolete files)

1. Click "Browse" 
2. Look to select a file, 
3. Switch to Finder
4. Switch back to FF

Result: File dialog is stuck open. Cancel doesn't work, Open doesn't work. I have to quit the browser.

Step 3 might or might not be necessary.

Screen recording:
https://www.youtube.com/edit?video_id=J6tP_b9GcdI&video_referrer=watch
I closed the window from where the browse button originated. It's a private website (artistworks) that requires login. 

Also notice the scren menubar.
Can you reproduce this reliably? As in, if you use the browse button again on the same website, switch to finder and back, does the same thing happen again?
Flags: needinfo?(dhtmlkitchen)
FWIW I couldn't reproduce on Mac OS X using Nightly. I assume yours is the release version Firefox 45.

The video clearly shows broken behavior, but it can't be used in an "attack" so we don't need to keep it hidden.
Group: firefox-core-security
Hi Garrett Smith,

I'm moving your issue to Firefox - File Handling component. Activity Streams is an add-on and has nothing to do with your issue.

Thanks.
Component: Activity Streams: General → File Handling
Version: 45 Branch

Leak a file dialog box and force the user to force-quit the browser. — no security issue here?

I cannot describe any attack, but this sounds pretty bad to me. 

Thank you.
Flags: needinfo?(dhtmlkitchen)
I will give steps to reproduce this when I can. The issue described and shown in the video has happened to me twice to me.
spohl, this sounds similar to an issue we discussed recently. Any clues?
Flags: needinfo?(spohl.mozilla.bugs)
This does sound related to other issues we've been seeing. However, I've been unable to reproduce this. I've been using http://www.cs.tut.fi/~jkorpela/forms/file.html among others to try and reproduce but have had no success with release or nightly.

Garrett, are you able to reproduce this issue on http://www.cs.tut.fi/~jkorpela/forms/file.html? If not, is there another public site that reproduces this for you?
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(dhtmlkitchen)
I just accidentally reproduced this using Gmail non-basic verion attach file. I tried to reproduce it again but was unable to. 

Using Firefox 45.0.2

I had to use activity monitor to force-quit.

The user experience is blocked and hijacked. The user is prevented from any further action, and is not able to quit the browser. To quit, the user must launch Activity Monitor. A lot of users won't know how to do that, and will feel helpless and stuck. 

If this problem can be triggered and reproduced, even if only some of the time, the triggering page can be used as an attack to demand that the user perform some action such as calling a phone number to provide credit card details, etc. 

I don't know how to consistently reproduce this problem.
Keywords: steps-wanted
I'm not sure if the dialog priority causes this issue. It seemed to me that I run into a similar situation.
When background popped up a alert, the Open dialog was hang there.
So, I suggest reporter to check if any alert dialog causes this issue.
Thanks. Have a nice day!

Attach the demo video (OPEN_FILE_DIALOG_STUCK-SITUATION02.swf)
* You can open it by using Firefox.
I can't open that SWF. I don't have flash on my system. 

I did not see any alert or other dialog boxes besides the Open File box. By "check" did you mean look? If not, please detail.

Thank you,
Flags: needinfo?(dhtmlkitchen)
Trivial STR:

- open browser console. Make sure you can evaluate script in it (open web console, settings, tick 'enable browser and add-on debugging toolboxes').
- run:

setTimeout(() => Services.prompt.alert(window, "Hi", null), 5000);

- switch back to the main window and hit Cmd-O (or use one of the testcases that has a browse / <input type=file> button)

- wait for the setTimeout to hit.


So basically, if any other thing runs an event on the main thread that causes us to pop up a sheet-alert, while the file opening dialog is open, stuff breaks.

:spohl, does this help you investigate further?
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to :Gijs Kruitbosch from comment #14)
> Trivial STR:

Just to be clear: these are trivial in the sense that they are contrived, but simple to follow. I expect that "in the wild", like in the swf screencast William attached, we show a slow script dialog or some other dialog for unrelated reasons, and inadvertently make the browser hard to interact with for the user in question.
Thanks for Gijs's help!:)

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Hi, Stephen,

Good day.
I was wondering if you have any comment on this bug?
Thank you!
Flags: needinfo?(spohl.mozilla.bugs)
Summary: Open File Dialon Box Stuck → Open File Dialog Box Stuck
Can this be reproduced with latest nightly? Even with the steps in comment 14 I can't reproduce the problem with a local build off of current trunk.
Flags: needinfo?(whsu)
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Stephen A Pohl [:spohl] from comment #17)
> Can this be reproduced with latest nightly? Even with the steps in comment
> 14 I can't reproduce the problem with a local build off of current trunk.

I just tried with nightly from yesterday, on OSX 10.11, and I can reproduce using the cmd-O option to open the filepicker. I haven't tried the input type=file case myself, but I would expect it to work the same way. You can still move the filepicker out of the way, accept the modal dialog, and then the filepicker can be interacted with again - but the filepicker typically appears (and stays) on top of the modal dialog, so that is not at all obvious.

Does that help?
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #18)
> (In reply to Stephen A Pohl [:spohl] from comment #17)
> > Can this be reproduced with latest nightly? Even with the steps in comment
> > 14 I can't reproduce the problem with a local build off of current trunk.
> 
> I just tried with nightly from yesterday, on OSX 10.11, and I can reproduce
> using the cmd-O option to open the filepicker. I haven't tried the input
> type=file case myself, but I would expect it to work the same way. You can
> still move the filepicker out of the way, accept the modal dialog, and then
> the filepicker can be interacted with again - but the filepicker typically
> appears (and stays) on top of the modal dialog, so that is not at all
> obvious.
> 
> Does that help?

Ah, yes, I thought we were talking about completely stuck and that there was no way to dismiss the modal dialog. Thanks! Will have another look tomorrow. Keeping n-i set.
Flags: needinfo?(whsu)
Ok, using mozregression it looks like this bug has been around forever. However, there is a point in time when things have improved. We used to completely hang the browser where neither the modal dialog nor the open file dialog could be dismissed. This changed and we can now switch back to the browser, dismiss the modal dialog and then dismiss the open file dialog. This occurred here:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20ca234fd62b&tochange=4b6d63b05a0a

And my money is on bug 996848 as the reason why things have improved.

To make sure we have the same expectations for a fix: should the fix be to not steal away focus from the open file dialog, allowing it to be dismissed? Or should it lose focus, but continue to be focusable and dismissable once regaining focus?
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Stephen A Pohl [:spohl] from comment #20)
> To make sure we have the same expectations for a fix: should the fix be to
> not steal away focus from the open file dialog, allowing it to be dismissed?
> Or should it lose focus, but continue to be focusable and dismissable once
> regaining focus?

I'm not a UX person, so I'm not the best person to answer this question. On Windows, these dialogs are fully modal, so it is impossible for another modal to come up while the filepicker is open. Testing Chrome on OS X, the alert comes up as a non-sheet modal dialog *on top of* the (sheet'd) filepicker, so you can see it and it is clear that that modal is preventing you from interacting with the filepicker until you accept the modal. I think that might be the most obvious interaction here, but I don't know if that's hard to implement. Out of the two options you describe I guess I would prefer the former, although it would also be nice that if it did end up losing focus it would continue to be focusable and dismissable once you focused it again. Really, any of these three options sound better than the current situation (if to different degrees). Does that help? You could ask UX people like :bram or :shorlander for advice if you think that would be more helpful than my ramblings. :-)
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch WIP (obsolete) (deleted) — Splinter Review
Modal dialogs have a long history (read: many, many bugs to read through) in our code base on OSX and I'm just starting to wrap my head around it. But it appears that when we're app modal (file picker), we can still have a nested loop that tries to display a window modal dialog (alert dialog). I couldn't find an existing way for nsWindowWatcher::OpenWindowInternal to know whether we're app modal, and my patch here is just a suggestion. Calling SetFakeModal when we're app modal still steals the focus away from the file picker, but the file picker remains app modal and only the file picker can be interacted with until it is dismissed. Aside from the initial loss of focus, things seem to be working as expected in my testing.

Markus, is this a direction worth pursuing?
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8784382 - Flags: feedback?(mstange)
Priority: -- → P1
Whiteboard: tpi:+
Blocks: 1297499
FWIW, this patch fixes the issue I was hitting in bug 1297499. I personally run into this bug at least twice a week, since I upload large files to Air Mozilla and YouTube for my livehacking.
Comment on attachment 8784382 [details] [diff] [review]
WIP

Review of attachment 8784382 [details] [diff] [review]:
-----------------------------------------------------------------

Modal dialogs are a part of widget/cocoa that I have been lucky enough to avoid so far, so I don't really know much about them. Since this patch helps, I think it's worth pursuing.

I think it's possible to come up with a better API than the one in this patch though. For example, overloading nsresult for a boolean return value isn't that great. And having a platform-specific workaround in cross-platform code isn't great either. As a suggestion, could you instead add a method like nsIWidget::OnOpeningModalChild, which does "if (isrunningappmodel) { this->setfakemodal(true); }" or something like that?
Attachment #8784382 - Flags: feedback?(mstange) → feedback+
Attached patch Patch (obsolete) (deleted) — Splinter Review
(In reply to Markus Stange [:mstange] from comment #25)
> Comment on attachment 8784382 [details] [diff] [review]
> I think it's possible to come up with a better API than the one in this
> patch though. For example, overloading nsresult for a boolean return value
> isn't that great.

Yes, definitely. That was not meant to make it into a final patch. Changed to bool.

> And having a platform-specific workaround in
> cross-platform code isn't great either. As a suggestion, could you instead
> add a method like nsIWidget::OnOpeningModalChild, which does "if
> (isrunningappmodel) { this->setfakemodal(true); }" or something like that?

I couldn't quite figure out how to make this work. However, nsIWidget already seems to have platform specific methods, such as SetFakeModal and RoundsWidgetCoordinatesTo that get overridden by Cocoa's widget/cocoa/nsWidgetCocoa.[h|mm]. In light of this, is this patch acceptable? Or would you like me to go back and perform more surgery?
Attachment #8784382 - Attachment is obsolete: true
Attachment #8788913 - Flags: review?(mstange)
Comment on attachment 8788913 [details] [diff] [review]
Patch

Review of attachment 8788913 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIWidget.h
@@ +652,5 @@
>          SetModal(aModal);
>      }
>  
>      /**
> +     * Are we app modal on Cocoa.

Maybe I should change this comment to be more cross-platform, something like:

> Are we app modal. Currently only implemented on Cocoa.
Comment on attachment 8788913 [details] [diff] [review]
Patch

Review of attachment 8788913 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIWidget.h
@@ +652,5 @@
>          SetModal(aModal);
>      }
>  
>      /**
> +     * Are we app modal on Cocoa.

Yes, the proposed comment sounds better.
Attachment #8788913 - Flags: review?(mstange) → review+
Attached patch Patch (deleted) — Splinter Review
Thanks, Markus! Changed comment, carrying over r+.
Attachment #8788913 - Attachment is obsolete: true
Attachment #8789485 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceeb69dfe4a5b7347a7d74a50f6fb5514ad2b1ba
Bug 1260850: Ensure that FilePicker (and other native app modal dialogs) on OSX remains responsive when a window modal dialog opens in the background. r=mstange
https://hg.mozilla.org/mozilla-central/rev/ceeb69dfe4a5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Do we know if this bug is on 51 only? Is this something we should uplift? (Dunno how safe that is)
Flags: needinfo?(spohl.mozilla.bugs)
Per comment 20, this is not a regression and behavior has actually slightly improved over time. Personally, I would prefer letting this change ride the trains and give us as much time as possible to catch any possible fallout.
Flags: needinfo?(spohl.mozilla.bugs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: