webapk: Avoid overwriting default CreationParams

webapk: Avoid overwriting default CreationParams

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

[boliu, 2017-02-22 00:01:10]

Description was changed from

==========
webapk: Avoid overwriting default CreationParams

BUG=664530
==========

to

==========
webapk: Avoid overwriting default CreationParams

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

[boliu, 2017-02-23 00:51:29]

boliu@chromium.org changed reviewers:
+ creis@chromium.org, tedchoc@chromium.org

[boliu, 2017-02-23 00:51:30]

This is part 2. Part 1 review hasn't started yet so no rush yet I guess. And I
still need to write a CL description.

creis for content
ted for chrome

This is all just plumbing a single int through all the layers, WebapkActivity ->
WebContents -> ChildProcessLauncher.

Left a few question comments as well.

https://codereview.chromium.org/2706933003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
(right):

https://codereview.chromium.org/2706933003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:246:
boolean incognito, boolean initiallyHidden, int childProcessParamId) {
so this s a general pattern I followed here. All warmup/prerender all use the
default params (because in general there's no corresponding activity when they
are created). I hope this forces the caller to make a choice about what the
childProcessParamId is.

https://codereview.chromium.org/2706933003/diff/60001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2706933003/diff/60001/content/browser/site_in...
content/browser/site_instance_impl.cc:162:
browsing_instance_->child_process_param_id());
I think this is the thing being discussed in the bug.

So subframes by default inherit from parent (so webapk subframe would be in
webapk). The other choice was to always have subframes always under chrome, in
which case should use default here.

https://codereview.chromium.org/2706933003/diff/60001/content/public/browser/...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/2706933003/diff/60001/content/public/browser/...
content/public/browser/site_instance.h:147: int child_process_param_id = 0);
I was wary to include child_process_launcher here to get the 0. Do you think
that include is fine? Or maybe DEFAULT_PARAM_ID should be declared here? Or
something else?

[boliu, 2017-02-24 19:12:14]

part 1 content/ got approved, and chrome/ is waiting on ted to come back

so could start reviewing content/ here as well now :p

[Charlie Reis, 2017-02-25 00:42:50]

Description was changed from

==========
webapk: Avoid overwriting default CreationParams

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

to

==========
webapk: Avoid overwriting default CreationParams

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

[Charlie Reis, 2017-02-25 00:45:48]

[CC nick, nasko, alexmos]

On 2017/02/24 19:12:14, boliu wrote:
> part 1 content/ got approved, and chrome/ is waiting on ted to come back
> 
> so could start reviewing content/ here as well now :p

We should find a different way to do this, because SiteInstance and
BrowsingInstance shouldn't have to know about an opaque, hard to
understand/document child_process_param_id, plus there are bigger implications
to customizing process creation.

The challenge appears to be that there aren't many ways to customize
RenderProcess creation based on context, such as knowing a property about the
WebContents it's for.  If your case can't be done in terms of BrowserContext,
then we may need a different, more generalizable way of customizing the renderer
process.  This is not easy-- as you mention in https://crbug.com/#c17, the
process reuse logic in RenderProcessHostImpl makes assumptions about which
processes are safe to reuse, and customizations like this would violate them. 
We would indiscriminately reuse processes without respect to the customizations,
once we're over the process limit.  I missed that implication at the time of
your comment, but seeing it in code makes it clear it's a serious problem.  I
don't think https://crbug.com/611203 would be sufficient.

(I'll also point out that BrowsingInstance != WebContents, if you're trying to
make it WebContents-specific as noted in comment 11 of the bug. 
BrowsingInstance includes popups and navigations that target new tabs as well. 
Not sure if you want those cases to share the customized behavior or not.)

One option: have you looked at RenderViewHostImpl::ComputeWebKitPrefs?  This is
one place that we send a set of preferences down to the renderer, and (for
better or worse) it's not perfectly identical across all processes, and could
take into account something about the RVH or WebContents.  (We apparently do
extension-specific things here based on the SiteInstance's site URL, for
example.)  This still faces the risks with process reuse, but at least it's in
one place and we could try to tackle the reuse problem for everything at once. 
Would it be sufficient for what you need?

[boliu, 2017-02-25 00:55:11]

On 2017/02/25 00:45:48, Charlie Reis (slow) wrote:
> [CC nick, nasko, alexmos]
> 
> On 2017/02/24 19:12:14, boliu wrote:
> > part 1 content/ got approved, and chrome/ is waiting on ted to come back
> > 
> > so could start reviewing content/ here as well now :p
> 
> We should find a different way to do this, because SiteInstance and
> BrowsingInstance shouldn't have to know about an opaque, hard to
> understand/document child_process_param_id, plus there are bigger implications
> to customizing process creation.
> 
> The challenge appears to be that there aren't many ways to customize
> RenderProcess creation based on context, such as knowing a property about the
> WebContents it's for.  If your case can't be done in terms of BrowserContext,
> then we may need a different, more generalizable way of customizing the
renderer
> process.  This is not easy-- as you mention in https://crbug.com/#c17, the
> process reuse logic in RenderProcessHostImpl makes assumptions about which
> processes are safe to reuse, and customizations like this would violate them. 
> We would indiscriminately reuse processes without respect to the
customizations,
> once we're over the process limit.  I missed that implication at the time of
> your comment, but seeing it in code makes it clear it's a serious problem.  I
> don't think https://crbug.com/611203 would be sufficient.
> 
> (I'll also point out that BrowsingInstance != WebContents, if you're trying to
> make it WebContents-specific as noted in comment 11 of the bug. 
> BrowsingInstance includes popups and navigations that target new tabs as well.

> Not sure if you want those cases to share the customized behavior or not.)
> 
> One option: have you looked at RenderViewHostImpl::ComputeWebKitPrefs?  This
is
> one place that we send a set of preferences down to the renderer, and (for
> better or worse) it's not perfectly identical across all processes, and could
> take into account something about the RVH or WebContents.  (We apparently do
> extension-specific things here based on the SiteInstance's site URL, for
> example.)  This still faces the risks with process reuse, but at least it's in
> one place and we could try to tackle the reuse problem for everything at once.

> Would it be sufficient for what you need?

I'm gonna paste this over to the bug, and continue discussion there. Important
to have yaron on the discussion.

And I'll check if something like ComputeWebKitPrefs works.

[boliu, 2017-04-28 19:06:52]

boliu@chromium.org changed reviewers:
- creis@chromium.org, tedchoc@chromium.org