I think the ultimate fix should be in the renderer side to detect such malicious page as it fires tons of request for the same URL.
Once all those requests reaches the browser, it will definitely slow everything down. 
Download uses DownloadRequestLimiter to throttle download, but even if user doesn't accept the prompt dialog, the number of request to handle on the browser side can still consume all the CPUs.

From robliao@ offline :

"""
I did some targeted lightspeeds (e.g. no-oping selected areas) in the downloads code on the UI thread on where we spend a bit of time and the resulting effect seems to be to make more time for the incoming requests with limited improvement in responsiveness. 

Rate-limiting the downloads as well as splitting the incoming tasks to the UI thread would also help in preventing this hang.
"""

The ultimate fix can't be strictly in the renderer IMO as our security model is for the browser to assume the renderer has gone rogue.

Having SequenceManager on the UI and IO threads will allow us to solve this nicely IMO.

If there isn't anything we can do to hotfix downloads, feel free to reassign to me.

The examples I have seen are using the download attribute, which could mean it was probably fixed by https://chromium-review.googlesource.com/758236 and reintroduced by https://chromium-review.googlesource.com/992322

Interesting, @jochen any idea if r548465 could have caused this?

it didn't cause this, but as carewolf@ said reintroduced.

I wonder whether https://chromium-review.googlesource.com/c/chromium/src/+/899666 is related?

At this point, this extra hop is no longer required

/cc mek

i don't think there are any hotfixes we can do at download side.
DownloadResourceThrottle already provides such protection by allowing only one download to take place from the page.  
However, if user allows multiple download to happen on the page, then the UI thread will become swamped by all the DownloadShelf updates. 

@qinmin : did you run the repro? It hangs even if user blocks multiple downloads. The whole UI is hung, even the page unresponsive dialog won't draw (see it as a transparent window in the taskbar).

Yes, I run it on my linux desktop. 
If user blocks multiple downloads, i can switch to another tab and open settings etc.
If user don't do anything, a pop up dialog will be shown, and the whole browser become unresponsive and switching tabs are not possible. I can still go to terminal to kill the browser process though.
If user allows multiple download, the download shelf will go crazy and the whole machine starts becoming sluggish. Shutting down the browser process also becomes a uneasy task.

If we don't want too much stress on the UI thread, we have to move a lot DownloadRequestLimiter work onto the IO thread. But since content settings/webcontents/NavigationHandle are all accessed on the UI thread, there is a lot of stuff needs to be reworked. I don't think a short term hotfix will be possible though.

Ok, would some sort of backpressure system on the download IPC work?

Le jeu. 5 juill. 2018 16 h 54, qinmin via monorail <
monorail+v2.951229368@chromium.org> a écrit :

Yes, if we can rate limit calls to RenderFrameMessageFilter::DownloadUrl(), that would definitely free things up on the UI thread

Agreed. The easiest solution at the moment is to impose a rate limit (akin to a QPS limit) on multiple download attempts.

ok, assigning this to me as i will work on a temporary rate limit fix

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6

commit a13cb2a1020b933c070d9db84fbc48dae5b0e1c6
Author: Min Qin <qinmin@chromium.org>
Date: Wed Jul 11 04:53:36 2018

Throttle url download requests from a RenderFrame

RenderFrameImpl::DownloadUrl() can be abused to trigger a large number
of download to overwhelm the browser process.
This CL rate limits the number of download to a maximum of 10 per second.

Bug:  860045 
Change-Id: I490ae3a99eb59aef709b9f1e6be5eb0f787354f4
Reviewed-on: https://chromium-review.googlesource.com/1132485
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574078}
[modify] https://crrev.com/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6/content/renderer/render_frame_impl.h
[modify] https://crrev.com/a13cb2a1020b933c070d9db84fbc48dae5b0e1c6/content/renderer/render_frame_impl_browsertest.cc

tested on an very old windows 7 machine (bought in 2009) with the latest canary build, browser is still responsive even after i clicked accept all download button from the malicious page.

This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

How safe is this merge overall?

Should be quite safe, i think. If there are pages that issuing a lot of downloads within a second, they will be affected anyways when M69 is released.

Approving merge to M68. Branch:3440

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d44d2be0eb693d9b477b476072011cc3b852a78

commit 4d44d2be0eb693d9b477b476072011cc3b852a78
Author: Min Qin <qinmin@chromium.org>
Date: Tue Jul 17 20:41:16 2018

Throttle url download requests from a RenderFrame

RenderFrameImpl::DownloadUrl() can be abused to trigger a large number
of download to overwhelm the browser process.
This CL rate limits the number of download to a maximum of 10 per second.

Bug:  860045 
Change-Id: I490ae3a99eb59aef709b9f1e6be5eb0f787354f4
Reviewed-on: https://chromium-review.googlesource.com/1132485
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#574078}(cherry picked from commit a13cb2a1020b933c070d9db84fbc48dae5b0e1c6)
Reviewed-on: https://chromium-review.googlesource.com/1140457
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#703}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/4d44d2be0eb693d9b477b476072011cc3b852a78/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/4d44d2be0eb693d9b477b476072011cc3b852a78/content/renderer/render_frame_impl.h
[modify] https://crrev.com/4d44d2be0eb693d9b477b476072011cc3b852a78/content/renderer/render_frame_impl_browsertest.cc