FTP login and error popups are blocking (i.e. make the main window unusable).

By causing a lot of FTP popups a site can practically make the browser unusable. I'll attach a video showing how this looks.

This is a simple setup with an FTP server allowing anoynmous login (I use proftpd, every one should do) and an HTML file containing lots of img tags loading images from an invalid directory.

I create the HTML with this:
for i in $(seq 1 10000); do echo "<img src='../$i/$i.png'>"; done

In this example I get a mixture of directory errors and login fields. I don't know exactly why the login fields show up, I guess the FTP server reacts somehow to being overloaded with requests.

There's no other option than killing the browser to escape such a situation. It's "only a DoS", but a pretty annoying one.

(Ceterum Censeo FTP esse delendam.)

Thanks for reporting!

This doesn't seem like a known DOS to me. Of course the auth dialog is already infamous (bug 377496) but I suppose this isn't strictly the same issue. I'll add it to the list.

bug #1362050 is also related.

(Shared underlying problem is that FTP is allowed to create window modal popups.)

(In reply to Hanno Boeck from comment #2)

bug #1362050 is also related.

(Shared underlying problem is that FTP is allowed to create window modal popups.)

Digging one level the deeper the problem, IMO, is that anything untrusted is allowed to create window modal popups, and we are planning to change that but it's harder than one might imagine.

Before I forget: I plan to disclose this 90 days from now if no fix is shipped earlier.

Assigning to Gijs because he wrote a patch.

https://hg.mozilla.org/integration/autoland/rev/2ce5cbe9bdf2d173dd19771adbfe3a5d43e7e639
https://hg.mozilla.org/mozilla-central/rev/2ce5cbe9bdf2

I'm tempted to suggest we uplift this to beta, as it's a straightforward enough change and there's a lot of beta bake time left. Dan, Valentin, what do you think?

Note that an uplift patch minus the string changes would need to be created if you want to nominate this for Beta.

Sure, this looks fine to uplift. Since the only "string change" is deleting one it would be trivial to leave netwerk/locales/en-US/necko.properties untouched. Won't hurt anything to have an unnecessary string in it.

(In reply to Daniel Veditz [:dveditz] from comment #10)

Sure, this looks fine to uplift. Since the only "string change" is deleting one it would be trivial to leave netwerk/locales/en-US/necko.properties untouched. Won't hurt anything to have an unnecessary string in it.

Agreed, I think that would be good.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

n/a

User impact if declined

Annoying FTP alerts

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

Yes

If yes, steps to reproduce

See comment #0

List of other uplifts needed

n/a

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This just removes all the alerts.

String changes made/needed

No (just leaving the dead string around in this patch)

Andrei, can your team verify before uplift?

I’ve managed to reproduce this issue, using the information provided in comment 0, with Nightly 67.0a1 (2019-01-28) on Windows 10 x64.
On my end, this is still reproducible with the latest Nightly 67.0a1 (2019-02-04), popups can’t be dismissed, browser gets unusable.
In order to create the FTP server, I used FileZilla.

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #14)

I’ve managed to reproduce this issue, using the information provided in comment 0, with Nightly 67.0a1 (2019-01-28) on Windows 10 x64.
On my end, this is still reproducible with the latest Nightly 67.0a1 (2019-02-04), popups can’t be dismissed, browser gets unusable.

What popups are you seeing? Can you create a screencast?

Sorry for the late response (time zone inconvenience). Here is the screencast with the issue on the latest Nightly.

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #16)

Created attachment 9041400 [details]
screencast issue.gif

Sorry for the late response (time zone inconvenience). Here is the screencast with the issue on the latest Nightly.

OK. So only the auth prompt issue remains; I didn't intend to fix that in this bug, only the alerts (that have nothing but text and an OK button). However, I would have expected bug 377496 to address the auth prompts. Can you file a separate bug for the auth prompts, make it block this bug and bug 377496, and needinfo :johannh ? Thank you.

I will mark this issue as verified fix on Fx 67, since it targets only the FTP alerts segment (no longer reproducible on the latest Nightly build) and the remaining concern related to auth prompts problem is treated separately in bug 1525267.

Gijs, should we still uplift this patch on its own? Just making sure we don't need to wait for a fix for bug 1525267.

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #19)

Gijs, should we still uplift this patch on its own? Just making sure we don't need to wait for a fix for bug 1525267.

I think the patch should probably be uplifted now given it's an improvement over the status quo, it also fixes bug 1362050, and because I'm confident Johann will get to 1525267 "soon". :-)

https://hg.mozilla.org/releases/mozilla-beta/rev/0630d18a7d26

The FTP alerts no longer occur with Beta 66.0b6 on Windows 10 x64. I had some difficulties in setting the proper environment on Mac. Can I consider this issue verified, although I can confirm the fix only on one platform?

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #23)

The FTP alerts no longer occur with Beta 66.0b6 on Windows 10 x64. I had some difficulties in setting the proper environment on Mac. Can I consider this issue verified, although I can confirm the fix only on one platform?

Yes, the ftp and alert implementation is shared, and the patch is basically just code removal, so this should be good now. Thanks!