User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0 Build ID: 20180327223059 Steps to reproduce: The attached HTML contains a script that will recursively open file selection dialogs blocking pretty much all kind of user interaction on the current Firefox window (until the OOM killer kills Firefox or something else?). This was tested on Arch Linux with Gnome 3. Actual results: Firefox "hangs". Expected results: Firefox doesn't wait till the user selects a file but just keep instantiating file selection dialogs over and over. On Chromium, for instance, this script will be stopped at the 5th dialog.

By the way, this also affects Firefox for Android: after a few seconds, some black screen and flickering, I get a message saying "Android System isn't responding. Do you want to close it?". After touching "OK" Firefox gets closed. This could be pretty annoying since when you restart Firefox you start in the same exploit page (on my example you need user interacting, but I guess this could be done as soon as the script loads making the app unusable maybe?). Thanks.

Different variations of this have been reported in the past (bug 1332590, bug 682565) but I think we're aiming to solve it all this summer through implementing a permission prompt for multiple downloads in bug 1306334. We should still keep the different variations open to make sure we test them after that's done...

This doesn't seem to be related to download spam, so I'm removing the dependency. It IS an interesting bug though. I'm not sure what the right fix is, here. I'd love to take this bug but I'll probably need mentorship from someone to figure out where to attempt to stop the "recursion". - I guess we could just block file inputs from being "activated" while one is already "active" (waiting for the user to pick a file). - Is there a legitimate use case for multiple file inputs activating at the same time? Doesn't seem like it.

Ah, right, I misread, this is about the file open dialog and not about "Save file". Thanks for correcting that. You could prevent non-user input using https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/dom/events/EventStateManager.cpp#4143 on opening file inputs, so somewhere about here: https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/dom/html/HTMLInputElement.cpp#3938. I'm not really sure about the web compat implications for this or whether this change is big enough to need an announcement or a pref. Maybe yes, considering that there's stuff like https://developer.mozilla.org/en-US/docs/Web/API/File/Using_files_from_web_applications#Using_hidden_file_input_elements_using_the_click()_method Smaug probably has thoughts on this.

Try modifying my example by adding input.click() at the end of the script: Firefox will block all those file dialogs because the click event was not generated by the user. Maybe some of that functionality could be used to prevent this kind of "popup bombs".

if type=file is click()'ed inside some user input, the file picker is supposed to be opened. Web depends on that behavior. I guess we should just prevent opening a new picker when there is another one open already for that particular browsing context tree. (so, add a flag to the top level docshell that file picker is open?)

Here a different approach than what I suggested for bug 675574: If we count how many popup blockers we have on stack, we can allow 1 popup for the whole chain. The last one resets the popup counter.

Comment on attachment 9030217 [details] [diff] [review] filePicker.patch >+/* static */ bool nsContentUtils::CanShowPopupByCount() { >+ MOZ_ASSERT(sPopupStatePusherCount); >+ >+ if (sPopupAllowedCount == 0) { >+ ++sPopupAllowedCount; >+ return true; >+ } >+ >+ return false; >+} This API is hard to understand. Why does a method called "CanDoFoo" increase some counter so that two consecutive method calls return different values? And the method isn't documented at all. Wouldn't something like TryUsePopupOpeningToken() or some such hint more what the method actually does?

Comment on attachment 9030275 [details] [diff] [review] part 1 - patch >+/* static */ bool nsContentUtils::TryUsePopupOpeningToken() { >+ MOZ_ASSERT(sPopupStatePusherCount); >+ >+ if (sPopupAllowedCount == 0) { >+ ++sPopupAllowedCount; >+ return true; >+ } >+ >+ return false; >+} >+ > static bool JSONCreator(const char16_t* aBuf, uint32_t aLen, void* aData) { > nsAString* result = static_cast<nsAString*>(aData); > result->Append(static_cast<const char16_t*>(aBuf), > static_cast<uint32_t>(aLen)); > return true; > } > > /* static */ bool nsContentUtils::StringifyJSON( >@@ -10478,8 +10492,20 @@ static bool JSONCreator(const char16_t* > host.Find(".", false, startIndexOfCurrentLevel + 1); > if (startIndexOfNextLevel <= 0) { > return false; > } > host = NS_LITERAL_CSTRING("*") + > nsDependentCSubstring(host, startIndexOfNextLevel); > } > } >+ >+/* static */ void nsContentUtils::PopupStatePusherCreated() { >+ ++sPopupStatePusherCount; >+} >+ >+/* static */ void nsContentUtils::PopupStatePusherDestroyed() { >+ MOZ_ASSERT(sPopupStatePusherCount); >+ >+ if (!--sPopupStatePusherCount) { >+ sPopupAllowedCount = 0; >+ } >+} Wouldn't this all be easier to understand if sPopupAllowedCount was a boolean, sPopupAllowed. It would be false by default and set to true in PopupStatePusherCreated() if ++sPopupStatePusherCount == 1. It needs to be set back to false in nsContentUtils::PopupStatePusherDestroyed() if (!--sPopupStatePusherCount) { nsContentUtils::TryUsePopupOpeningToken() would then do something like bool retVal = sPopupAllowed; sPopupAllowed = false; return retVal; with that, r+

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2dc7c0e8c9 Allow just 1 FilePicker per event, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bf214bd2e6 Allow just 1 FilePicker per event - tests, r=smaug

https://hg.mozilla.org/mozilla-central/rev/3c2dc7c0e8c9 https://hg.mozilla.org/mozilla-central/rev/c4bf214bd2e6