Add experimental code behind a flag for Content Size Policy

Add experimental code behind a flag for Content Size Policy
TODO: add a description linking all the various documents.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[Charlie Harrison, 2016-07-25 15:44:30]

Description was changed from

==========
Add experimental code behind a flag for Content Size Policy

BUG=
==========

to

==========
Add experimental code behind a flag for Content Size Policy

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Harrison, 2016-07-25 15:45:27]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 15:45:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 16:05:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 16:05:39]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Harrison, 2016-07-25 16:19:26]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 16:19:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 17:03:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 17:03:38]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-07-25 23:14:53]

Matt, do you think you could take a high level look at this? The strategy here
is to keep "source of truth" data on the UI thread, and "approximate data" on
the IO thread.

Note: I expanded LoaderIOThreadNotifier in RDHI for this task. It is possibly
big enough to expand into its own class, but it shares some anon functions with
RDHI.

How frames are added to the map is a big TODO. Right now we just use a flag
value.

[Charlie Harrison, 2016-07-25 23:15:10]

csharrison@chromium.org changed reviewers:
+ mmenke@chromium.org

[Charlie Harrison, 2016-07-25 23:15:10]

actually adding Matt:
Matt, do you think you could take a high level look at this? The strategy here
is to keep "source of truth" data on the UI thread, and "approximate data" on
the IO thread.

Note: I expanded LoaderIOThreadNotifier in RDHI for this task. It is possibly
big enough to expand into its own class, but it shares some anon functions with
RDHI.

How frames are added to the map is a big TODO. Right now we just use a flag
value.

[Charlie Harrison, 2016-07-25 23:15:46]

Description was changed from

==========
Add experimental code behind a flag for Content Size Policy

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add experimental code behind a flag for Content Size Policy
TODO: add a description linking all the various documents.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[mmenke, 2016-07-27 17:42:32]

On 2016/07/25 23:15:10, csharrison wrote:
> actually adding Matt:
> Matt, do you think you could take a high level look at this? The strategy here
> is to keep "source of truth" data on the UI thread, and "approximate data" on
> the IO thread.
> 
> Note: I expanded LoaderIOThreadNotifier in RDHI for this task. It is possibly
> big enough to expand into its own class, but it shares some anon functions
with
> RDHI.
> 
> How frames are added to the map is a big TODO. Right now we just use a flag
> value.

Going to punt this one until I have my two big loader reviews wrapped up.  Just
been spending too much time reviewing loader code.

[Charlie Harrison, 2016-07-27 17:44:20]

On 2016/07/27 17:42:32, mmenke wrote:
> On 2016/07/25 23:15:10, csharrison wrote:
> > actually adding Matt:
> > Matt, do you think you could take a high level look at this? The strategy
here
> > is to keep "source of truth" data on the UI thread, and "approximate data"
on
> > the IO thread.
> > 
> > Note: I expanded LoaderIOThreadNotifier in RDHI for this task. It is
possibly
> > big enough to expand into its own class, but it shares some anon functions
> with
> > RDHI.
> > 
> > How frames are added to the map is a big TODO. Right now we just use a flag
> > value.
> 
> Going to punt this one until I have my two big loader reviews wrapped up. 
Just
> been spending too much time reviewing loader code.

SGTM I noticed you are quite busy. Too bad Randy is out.

[mmenke, 2016-07-28 15:11:26]

On 2016/07/27 17:44:20, csharrison wrote:
> On 2016/07/27 17:42:32, mmenke wrote:
> > On 2016/07/25 23:15:10, csharrison wrote:
> > > actually adding Matt:
> > > Matt, do you think you could take a high level look at this? The strategy
> here
> > > is to keep "source of truth" data on the UI thread, and "approximate data"
> on
> > > the IO thread.
> > > 
> > > Note: I expanded LoaderIOThreadNotifier in RDHI for this task. It is
> possibly
> > > big enough to expand into its own class, but it shares some anon functions
> > with
> > > RDHI.
> > > 
> > > How frames are added to the map is a big TODO. Right now we just use a
flag
> > > value.
> > 
> > Going to punt this one until I have my two big loader reviews wrapped up. 
> Just
> > been spending too much time reviewing loader code.
> 
> SGTM I noticed you are quite busy. Too bad Randy is out.

Why are you adding this stuff to the RDH?  Just so it can own the
content_size_manager_?

[Charlie Harrison, 2016-07-28 15:20:44]

Most of the RDH code is added because it already has and shares code with the
LoaderIOThread notifier. I'm happy to try to separate that out into a separate
file though.

[mmenke, 2016-07-28 15:33:12]

On 2016/07/28 15:20:44, csharrison wrote:
> Most of the RDH code is added because it already has and shares code with the
> LoaderIOThread notifier. I'm happy to try to separate that out into a separate
> file though.

I think we're adding way too much UI thread to to the RDH here, and too much to
it in general.  I haven't gone through this carefully enough to suggest an
alternative approach.

[Charlie Harrison, 2016-07-28 15:36:29]

Ok yeah I agree with you. I can upload a patch moving LoaderIOThread notifier to
another file. The only things it shares with the RDHI is some anonymous
functions which could be refactored.

[Charlie Harrison, 2016-08-01 18:33:11]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 18:33:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 19:43:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 19:43:37]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-08-01 21:36:04]

On 2016/07/28 15:36:29, csharrison wrote:
> Ok yeah I agree with you. I can upload a patch moving LoaderIOThread notifier
to
> another file. The only things it shares with the RDHI is some anonymous
> functions which could be refactored.

Is this ready for review?

[Charlie Harrison, 2016-08-01 21:41:56]

On 2016/08/01 21:36:04, mmenke wrote:
> On 2016/07/28 15:36:29, csharrison wrote:
> > Ok yeah I agree with you. I can upload a patch moving LoaderIOThread
notifier
> to
> > another file. The only things it shares with the RDHI is some anonymous
> > functions which could be refactored.
> 
> Is this ready for review?

Yeah feel free to start reviewing. Note: I deferred writing tests until I had
high level sign off. Once you think this approach is reasonable I'll make this
production ready.

[mmenke, 2016-08-02 18:02:12]

Just trying to get a feel for this CL.  I still really don't like that the
LoaderIOThreadNotifier is doing things other than notify the IO thread of stuff,
and will suggest changing that at some point, just want to get a better handle
on the design considerations in play before that.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/content_size_resource_handler.cc (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/content_size_resource_handler.cc:52: frame_limits_[id] =
limit;
How useful is this?  This seems best-effort-y, given that we don't tie together
nested frames here.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.cc (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:63: if
(handle->IsInMainFrame())
A main frame navigation is a navigation "in" a main frame, right?  (As opposed
to an iframe that's directly a child of the main frame being a navigation
directly *within* a main frame).

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:80: NavigationHandle*
navigation_handle) {
Do we really need a NavigationThrottle for this?  Is this the only way to learn
when a frame's frame_routing_id/frame_tree_node has changed or something?

Or is it too hard to stop a subnavigation through other APIs, and the
ResourceHandlers aren't good enough alone?

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:82: // Only consider http/s
or about:blank urls for blocking.
Why do we care about about:blank?  Is there a spec?

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:133: WebContents* contents =
WebContents::FromRenderFrameHost(rfh);
By creating and killing iframes, and then re-requesting resources that are now
in the cache, could a renderer bypass this stuff?  It would, admittedly, have to
be very precise about the timing.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:138:
per_notifier_map[notifier][it.first] = it.second;
+=?  We can have multiple notifications for the same frame...I assume that's the
whole reason there are two loops here, and we don't call UpdateDataAccounting
here?

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:151: FrameSize* size =
&frame_tree_node_sizes_[node->frame_tree_node_id()];
Half this code uses frame_tree_node_ids, half uses child_id/route_id.  Could we
standardize on one or the other?  Is there a 1-to-1 correspondence between the
two?  If I navigate a frame, do both stay the same (At least in a non-OOPIF
world)?

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:187:
frame_size->content_left -= it.second;
This should update the content_left value of all frame_sizes that
GetEffectiveLimit goes through.

Just consider a subframe A with children B and C.  A has a limit of 10k, C a
limit of 5k.  B downloads 4k, which it subtracts from the 5k limit, but not the
10k limit.  C then downalods 8k.  It just checks this against the 10k limit
(Which the 4k was not subtracted from), and violates the limit.  It would be
easy to exploit this.

[mmenke, 2016-08-02 18:04:51]

Oh, and I have to ask once again:  Have you thought about ServiceWorker or SDCH
at all?  I'm not going to sign off on landing half an API implementation, unless
we know we can (And are going to) actually finish it.  A spec would also be
nice, if we don't have one.

[Charlie Harrison, 2016-08-02 18:35:37]

No new PS, just responding to comments.

Re: Serviceworker. Still unsure about that. I've raised the issue on github:
https://github.com/WICG/content-size/issues/2

Re: SDCH, I will talk to rdsmith@ about it because I don't really know the
context.

thanks for the review :)

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/content_size_resource_handler.cc (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/content_size_resource_handler.cc:52: frame_limits_[id] =
limit;
On 2016/08/02 18:02:11, mmenke wrote:
> How useful is this?  This seems best-effort-y, given that we don't tie
together
> nested frames here.

It's pretty useful. I don't exact numbers but I could get them.

Essentially, what we had before was a std::set instead of a map (set of ids to
check on UI thread). It was trivial to make this decently more robust by passing
the limit.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.cc (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:63: if
(handle->IsInMainFrame())
On 2016/08/02 18:02:11, mmenke wrote:
> A main frame navigation is a navigation "in" a main frame, right?  (As opposed
> to an iframe that's directly a child of the main frame being a navigation
> directly *within* a main frame).

Right. Now that you mention it, this is confusingly named :P.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:80: NavigationHandle*
navigation_handle) {
On 2016/08/02 18:02:11, mmenke wrote:
> Do we really need a NavigationThrottle for this?  Is this the only way to
learn
> when a frame's frame_routing_id/frame_tree_node has changed or something?
> 
> Or is it too hard to stop a subnavigation through other APIs, and the
> ResourceHandlers aren't good enough alone?

So, actually I think I will remove the throttle. I added it so we can cancel
main resource loads before they commit, but clamy@ will soon add a
ReadyToCommitNavigation callback to WCO. With that I think we have what we need.

For context, we don't know the routing id of a navigation until it is ready to
commit. We *could* send the provisional routing id, but that seemed like too
much complication. I think for right now I will go off of DidFinishNavigation,
with a TODO to change it to ReadyToCommit.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:82: // Only consider http/s
or about:blank urls for blocking.
On 2016/08/02 18:02:11, mmenke wrote:
> Why do we care about about:blank?  Is there a spec?

No spec yet :(

The only reason I added about:blank is because it's a common way to inject ads
into a page.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:133: WebContents* contents =
WebContents::FromRenderFrameHost(rfh);
On 2016/08/02 18:02:11, mmenke wrote:
> By creating and killing iframes, and then re-requesting resources that are now
> in the cache, could a renderer bypass this stuff?  It would, admittedly, have
to
> be very precise about the timing.

No, because we count bytes for resources loaded from the cache.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:138:
per_notifier_map[notifier][it.first] = it.second;
On 2016/08/02 18:02:11, mmenke wrote:
> +=?  We can have multiple notifications for the same frame...I assume that's
the
> whole reason there are two loops here, and we don't call UpdateDataAccounting
> here?

Acknowledged. Think you're right.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:151: FrameSize* size =
&frame_tree_node_sizes_[node->frame_tree_node_id()];
On 2016/08/02 18:02:11, mmenke wrote:
> Half this code uses frame_tree_node_ids, half uses child_id/route_id.  Could
we
> standardize on one or the other?  Is there a 1-to-1 correspondence between the
> two?  If I navigate a frame, do both stay the same (At least in a non-OOPIF
> world)?

*sigh* that's one of the complications here.

frame_tree_node_ids stay constant in a frame, regardless of OOPIF, cross process
navigation, anything. This means that our data accounting needs to use them for
the "golden" source of truth for per-frame accounting, otherwise an iframe could
just continually navigate cross process and reset its counts (and play a bit
more video, etc.).

The problem is that frame_tree_node_id is a UI thread thing, and routing id is
known on the IO thread. frame_tree_node_id is known on the IO thread for
navigations (post PlzNavigate), but that's it.

Possibly, we could add them wholesale to ResourceRequestInfoImpl, but I'm not
sure if other owners would be okay with that.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:187:
frame_size->content_left -= it.second;
On 2016/08/02 18:02:12, mmenke wrote:
> This should update the content_left value of all frame_sizes that
> GetEffectiveLimit goes through.
> 
> Just consider a subframe A with children B and C.  A has a limit of 10k, C a
> limit of 5k.  B downloads 4k, which it subtracts from the 5k limit, but not
the
> 10k limit.  C then downalods 8k.  It just checks this against the 10k limit
> (Which the 4k was not subtracted from), and violates the limit.  It would be
> easy to exploit this.

I think you mixed up B and C in your example, but I understand the point. This
is a necessary change.

[Charlie Harrison, 2016-08-02 18:44:43]

rdsmith@ told me that SDCH dictionaries do not go through RDH. We agreed that it
is reasonable to not count those requests against a subframe.

Do you think that's reasonable? Is it easy to exploit SDCH and force the browser
to download a huge dictionary?

[mmenke, 2016-08-02 18:56:35]

On 2016/08/02 18:44:43, csharrison wrote:
> rdsmith@ told me that SDCH dictionaries do not go through RDH. We agreed that
it
> is reasonable to not count those requests against a subframe.
> 
> Do you think that's reasonable? Is it easy to exploit SDCH and force the
browser
> to download a huge dictionary?

It has a dictionary limit of about 1MB.  I'm not familiar with vcdiff, but I
assume you can basically make it say "Use the entire dictionary as the response
body", probably with a very small file size.

So all an attacker needs to do is to load a resource over an XHR, the response
the first time (With no advertised dictionary) is tiny, claims it's uncacheable,
and advertises a huge dictionary.  Then the attacker waits for the dictionary to
download (Maybe polling the resource), and when the dictionary is downloaded,
then it can use the body as the response.

If we're counting decompressed bytes, I don't think this is a problem, and we
should be fine (Except against people who just want to make us download tons of
data for kicks, which isn't our thread model here).  If we were counting only
network bytes, I'd be very concerned about this, since we are concerned about
bad actors who want us to download more data and then actually do something with
it.

[Charlie Harrison, 2016-08-02 19:06:55]

On 2016/08/02 18:56:35, mmenke wrote:
> On 2016/08/02 18:44:43, csharrison wrote:
> > rdsmith@ told me that SDCH dictionaries do not go through RDH. We agreed
that
> it
> > is reasonable to not count those requests against a subframe.
> > 
> > Do you think that's reasonable? Is it easy to exploit SDCH and force the
> browser
> > to download a huge dictionary?
> 
> It has a dictionary limit of about 1MB.  I'm not familiar with vcdiff, but I
> assume you can basically make it say "Use the entire dictionary as the
response
> body", probably with a very small file size.
> 
> So all an attacker needs to do is to load a resource over an XHR, the response
> the first time (With no advertised dictionary) is tiny, claims it's
uncacheable,
> and advertises a huge dictionary.  Then the attacker waits for the dictionary
to
> download (Maybe polling the resource), and when the dictionary is downloaded,
> then it can use the body as the response.
> 
> If we're counting decompressed bytes, I don't think this is a problem, and we
> should be fine (Except against people who just want to make us download tons
of
> data for kicks, which isn't our thread model here).  If we were counting only
> network bytes, I'd be very concerned about this, since we are concerned about
> bad actors who want us to download more data and then actually do something
with
> it.

Thanks for the clarification. I say we proceed with speccing decrompressed bytes
to avoid this issue. We already count cached bytes so I think that's fine.

[mmenke, 2016-08-05 17:16:27]

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/content_size_resource_handler.h (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/content_size_resource_handler.h:32: class
ContentSizeResourceHandlerManager {
I'm not a fan of having two top-level classes in a single header.  It makes
navigation confusing.  Should either rename this file
content_size_resource_handler_manager, and move the ContentSizeResourceHandler
into an anonymous namespace in the cc file, or move them into separate files.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.cc (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:102:
base::StringToInt(policy_string, &content_size);
Should we be using int64_t here, or even uint64_t?

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:118: RenderFrameHost*
render_frame_host) {
This doesn't do anything.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.h (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.h:62: std::map<int, FrameSize>
frame_tree_node_sizes_;
This really doesn't belong in something called "LoaderIOThreadNotifier".  It has
nothing to do with notifying the IOThread of anything.

We need to store stuff on the UI thread, though, and learn about new frames
there.  Maybe a new WebContentsObserver, instead, specifically for this
component?

Alternatively, could make a separate class, and have this one own it can call
into it.

[mmenke, 2016-08-05 17:18:17]

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.cc (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:100:
switches::kContentSizePolicy);
Why are we getting this as a command line string...This is supposed to come from
an iframe parameter, I assume?

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:102:
base::StringToInt(policy_string, &content_size);
On 2016/08/05 17:16:27, mmenke wrote:
> Should we be using int64_t here, or even uint64_t?

Oh, and I don't think anyone will be setting a limit over 2 GB, but seems like
we may as well support it.

[Charlie Harrison, 2016-08-08 00:10:55]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 00:11:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 00:37:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 00:37:40]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Charlie Harrison, 2016-08-08 04:36:07]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 04:36:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 06:07:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 06:07:32]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-08-08 14:04:39]

Ok tried to respond to most comments. Thanks for taking a look.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/content_size_resource_handler.h (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/content_size_resource_handler.h:32: class
ContentSizeResourceHandlerManager {
On 2016/08/05 17:16:27, mmenke wrote:
> I'm not a fan of having two top-level classes in a single header.  It makes
> navigation confusing.  Should either rename this file
> content_size_resource_handler_manager, and move the ContentSizeResourceHandler
> into an anonymous namespace in the cc file, or move them into separate files.

SGTM moved to a separate file.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.cc (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:100:
switches::kContentSizePolicy);
On 2016/08/05 17:18:17, mmenke wrote:
> Why are we getting this as a command line string...This is supposed to come
from
> an iframe parameter, I assume?

Yes. We wanted to land an experiment that just applies a blanket policy to all
cross origin subframes while the API of the spec is worked out. I imagine the
final interface will end up having the ContentSizeFrameCounter be a mojo
interface that the renderer talks to when it sees that an iframe has a max-size
attribute. Then we will add frames to the size map in the mojo callback.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:102:
base::StringToInt(policy_string, &content_size);
On 2016/08/05 17:18:17, mmenke wrote:
> On 2016/08/05 17:16:27, mmenke wrote:
> > Should we be using int64_t here, or even uint64_t?
> 
> Oh, and I don't think anyone will be setting a limit over 2 GB, but seems like
> we may as well support it.

Yeah why not. Changing it to uint64_t sgtm. I had everything as int to keep
static_casting to a minimum in the resource handler.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:118: RenderFrameHost*
render_frame_host) {
On 2016/08/05 17:16:27, mmenke wrote:
> This doesn't do anything.

Sorry about that. Removed.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:138:
per_notifier_map[notifier][it.first] = it.second;
On 2016/08/02 18:35:37, csharrison wrote:
> On 2016/08/02 18:02:11, mmenke wrote:
> > +=?  We can have multiple notifications for the same frame...I assume that's
> the
> > whole reason there are two loops here, and we don't call
UpdateDataAccounting
> > here?
> 
> Acknowledged. Think you're right.

Done.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.cc:187:
frame_size->content_left -= it.second;
On 2016/08/02 18:35:36, csharrison wrote:
> On 2016/08/02 18:02:12, mmenke wrote:
> > This should update the content_left value of all frame_sizes that
> > GetEffectiveLimit goes through.
> > 
> > Just consider a subframe A with children B and C.  A has a limit of 10k, C a
> > limit of 5k.  B downloads 4k, which it subtracts from the 5k limit, but not
> the
> > 10k limit.  C then downalods 8k.  It just checks this against the 10k limit
> > (Which the 4k was not subtracted from), and violates the limit.  It would be
> > easy to exploit this.
> 
> I think you mixed up B and C in your example, but I understand the point. This
> is a necessary change.

Done.

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
File content/browser/loader/loader_io_thread_notifier.h (right):

https://codereview.chromium.org/2180933002/diff/40001/content/browser/loader/...
content/browser/loader/loader_io_thread_notifier.h:62: std::map<int, FrameSize>
frame_tree_node_sizes_;
On 2016/08/05 17:16:27, mmenke wrote:
> This really doesn't belong in something called "LoaderIOThreadNotifier".  It
has
> nothing to do with notifying the IOThread of anything.
> 
> We need to store stuff on the UI thread, though, and learn about new frames
> there.  Maybe a new WebContentsObserver, instead, specifically for this
> component?
> 
> Alternatively, could make a separate class, and have this one own it can call
> into it.

Fair point. I mainly stuffed this logic here because this was the class designed
to be glue between loader-related things on the UI/IO thread. I've added another
class: ContentSizeFrameCounter, that is another WebContentsObserver, and kept
this class as-is.

[mmenke, 2016-09-08 17:54:48]

Gah, is this one waiting on me?

[Charlie Harrison, 2016-09-08 17:58:16]

On 2016/09/08 17:54:48, mmenke wrote:
> Gah, is this one waiting on me?

Nope. Let's close this issue for now. Spec discussions are still ongoing and it
isn't clear what this will look like.