+yfriedman

This should be considered ship blocking for webapk, and I just saw your post it shipped today.

Summary is ChildProcessCreationParams is global, so it should only be set once on start up, and never changed again. But webapk is modifying it at run time, and I honestly don't know what could break.

Well naturally the issue is if somehow the params don't get unset when they should. This could result in a process erroneously being attributed to the webapk or chrome (when it should be the other). Unless there's something more sinister for monochrome where it could only possibly work with chrome's package? Oh from the CL it could crash a WebAPK on N? That said, we don't know of cases where this is actually broken. I agree it should be fixed, just unclear atm (getting back up to speed) of urgency.

I'd go with assume unsafe unless proven safe. I'm super uncomfortable because it's going to be very hard attributing a potential problem back to this since any potential problem will only show up on corner cases, or only reproduce racily, so hard to reproduce and pin down..

side note.. the child launching code probably needs a refactor, or if not, at least more comments and more tests. recently, every time someone (including me) makes a non-trivial change to that code, it introduces a bunch of crashes and then amineer pings me a week later

Have you started looking at this yet?

58 branch is in 2 weeks, and I've been working in refactoring childprocesslauncher recently. Want me to take it?

boliu@:
FYI, hanxi@ is OOO until March 5. It would be great if you want to take it :)

Alright, this is gonna be quite involved. Maybe shouldn't have signed up for this :p

This should be per-webcontents: activity isn't a high level concept in content. ChildProcessLauncher doesn't know anything that could be used to identify webapk processes; it isn't necessarily tied to a webcontents either. So probably can't use things like ContentBrowserClient. Could keep an per-activity map, and then try to plumb the activity into the child process launcher, but globals are gross and got us into this mess in the first place.

So the path to plumb ChildProcessCreationParams is..
WebApkActivity -> TabDelegateFactory -> Tab.initialize -> native WebContents(::CreateParams) -> SiteInstance(Impl) -> RenderProcessHostImpl -> ChildProcessLauncher(Helper|Android) -> java ChildProcessLauncher -> ChildProcessConnectionImpl

Unfortunately, forcing all launches to pass in a new param isn't really feasible. So semantic will be “custom CreationParams overrides the global one”. Launches with custom params needs to skip warmUp, which webapk doesn’t use.

Also found out that webapk shares the same BindingManager/ModerateBindingPool with chrome renderers, which makes no sense either. But that’s another bug for another day..

How do I test webapks easily? like with local builds?

These instructions should work:
https://sites.google.com/a/google.com/clank/engineering/feature-areas/webapks

Hmm. It's probably worth thinking through whether this should apply at WebContents or SiteInstance. I can imagine a subframe in a OOPIF world wanting to launch with chrome's renderer pool as opposed to the WebApk's. There's also the case where you can use a WebApk, then launch chrome and visit a url which technically matches the WebApk but since you typed it into the omnibox, we don't forward to the WebApk. Should that re-use the same renderer/site instance?

s/WebContents/top frame of webapk/. As I'm writing the CL, a lot of things changed from my original plan, and one of things is this will live on SiteInstance rather than WebContents. But I'm not done writing it yet, so..

> I can imagine a subframe in a OOPIF world wanting to launch with chrome's renderer pool as opposed to the WebApk's.

That's surprising to me. Subframe is loaded by webapk, why should it be in chrome's pool (other than the fact that webapk only has 3 renderer slots..)?

> There's also the case where you can use a WebApk, then launch chrome and visit a url which technically matches the WebApk but since you typed it into the omnibox, we don't forward to the WebApk. Should that re-use the same renderer/site instance?

I would expect that not to re-use the same SiteInstance today already.

Comments 14-15: I'm not familiar with WebApks, but it's certainly true that a WebContents in Chrome could have a different RPH/SiteInstance associated with a subframe.  (We're testing this soon on Chrome for Android for --top-document-isolation.)  That may not be an issue on its own for WebApks, though, unless there's some way for an OOPIF to create a WebApk that should inherit things about the OOPIF process.

Is it more of a question of whether you want to support OOPIFs inside of WebApks, and whether those will be allowed to use something like an existing TDI subframe process?  That's perhaps up to you, though keep in mind that you can't have different Profiles for different frames in the same page, if that matters.

> unless there's some way for an OOPIF to create a WebApk that should inherit things about the OOPIF process.

That's not possible.

> Is it more of a question of whether you want to support OOPIFs inside of WebApks, and whether those will be allowed to use something like an existing TDI subframe process?  That's perhaps up to you, though keep in mind that you can't have different Profiles for different frames in the same page, if that matters.

Right that's the question.

I think webapk lives in the same profile as the rest of the non-incognito tabs, so that should not be a problem.

That does raise another question for me though. The code in content that decides which process should host which frame isn't aware of webapk though, so there could be problems if webapk's subframes are allocated in webapk's package for reuse. If content decides to reuse an existing renderer for webapk, then chances are it will come from chrome's package. And the other way is possible too, that content decides to reuse a renderer from webapk for chrome.

I guess that's an argument for keeping all subframe processes under chrome? Or maybe we should teach content to not re-use renderers between webapk and chrome? Yaron?

Sounds like fixing issue 611203 may help here-- we're hoping to disable renderer reuse unless a WebContents is explicitly marked as ok with it.  That way we don't share normal tab renderers with special features like Contextual Search or WebApk.  (I don't have a timeline for that one, but does it sound relevant here?)

I want to say... Webapk renderers aren't special security-wise. All this plumbing to start the renderer under the "correct" package is purely for resource accounting purposes. So I guess the tradeoff of accurate accounting (ie never sharing renderer) comes at the cost of more resources. And we probably don't want to regress on resources for webapk.

But I'm only recently learning about webapk as well, so should wait for Yaron to adjudicate :p

Putting all subframes in chrome would definitely be easier to implement though (I think)

"I want to say... Webapk renderers aren't special security-wise. All this plumbing to start the renderer under the "correct" package is purely for resource accounting purposes. So I guess the tradeoff of accurate accounting (ie never sharing renderer) comes at the cost of more resources. And we probably don't want to regress on resources for webapk."

Ya, I can agree to that. 

"> Is it more of a question of whether you want to support OOPIFs inside of WebApks, and whether those will be allowed to use something like an existing TDI subframe process?  That's perhaps up to you, though keep in mind that you can't have different Profiles for different frames in the same page, if that matters.

Right that's the question.

I think webapk lives in the same profile as the rest of the non-incognito tabs, so that should not be a problem."

Correct

"That does raise another question for me though. The code in content that decides which process should host which frame isn't aware of webapk though, so there could be problems if webapk's subframes are allocated in webapk's package for reuse. If content decides to reuse an existing renderer for webapk, then chances are it will come from chrome's package. And the other way is possible too, that content decides to reuse a renderer from webapk for chrome.

I guess that's an argument for keeping all subframe processes under chrome? Or maybe we should teach content to not re-use renderers between webapk and chrome? Yaron?"

I dunno if we want to keep all of them in chrome. I think it's fine to let the original launcher bear the resource allocation (since it was responsible for launching the renderer) because I agree that this probabyl shouldn't result in additional resource load. 

CLs, in case anyone from webapk want to double check. I checked g+ webapk launches fine under the right package.
https://codereview.chromium.org/2705133002/
https://codereview.chromium.org/2706933003/

Moving code review discussion here. I think I cc-ed all the people added on the code review..

On 2017/02/25 00:45:48, Charlie Reis (slow) wrote:
> 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.

fair..

> 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.

So in #19/20, yaron and I decided that re-use is not a problem, as the only thing broken will be resource accounting. And accuracy is less important than not regressing resources. I guess this does make correctness hard to reason about though :/

> (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.)

I think BrowsingInstance is actually the right place then. Anything child spawned that's triggered by something in a webapk should stay within that webapk.

My "per webcontents" comment was wrong.

> 
> 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?

ContentBrowserClient::OverrideWebkitPrefs (or I guess something like it since this is not a webkit pref) would work if we ignore all the other potential child proceses spawned from webapk.

But I always hated code that "unwarps up layers of abstraction", like multistep code that goes from frame->host->webcontents->contentviewcore with a null check at each step (I just made that up, so might not be real). And looking at chrome code, I think will need to maintain a global map, rather than only following pointers.

So I think this more functional pass parameters down on construction is better. And can naturally also catch all the other child processes spawned from webapk.

btw, can we remove RVG since webapk is public now?

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

commit f2e4ff7579e19d3f48d3aca2513470e5afdff01f
Author: boliu <boliu@chromium.org>
Date: Tue Feb 28 01:00:19 2017

android: Allow registering multiple CreationParams

This is first step to avoid webapk overwriting default
CreationParams at run time. For now it's only used in
tests, and native side just hard codes the default ID.

Also added a check that warm up connection cannot be
used if the creation params do not match.

Other clean ups:
* Hide methods that don't need to be public.
* Remove copy. Params is already immutable.
* Remove redundant registerDefault call from
  Activities. Monochrome application already registers
  default in its onCreate.

BUG= 664530 

Review-Url: https://codereview.chromium.org/2705133002
Cr-Commit-Position: refs/heads/master@{#453428}

[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/chrome/android/java/templates/MonochromeApplication.template
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/content/browser/android/child_process_launcher_android.cc
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/content/public/android/java/src/org/chromium/content/browser/ChildProcessCreationParams.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
[modify] https://crrev.com/f2e4ff7579e19d3f48d3aca2513470e5afdff01f/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java

creis: any more feedback on #22?

I thought about OverrideWebKitPrefs a bit. If we add a new method to ContentBrowserClient, then what parameter do we pass up to the embedder?

If we pass RenderProcessHost, then it's pretty much useless because embedder can't map that back to a particular WebContents.

If we pass up something like RVH/RFH/WC, then I think we get into the same "inconsistency" issue we've been discussing, that sometimes a process is re-used, and then embedder won't get a chance to override anything.

Sorry for the delay-- too much going on this week.

Comments 22/25: It does sound like you want something effectively scoped to the BrowsingInstance (which would include cross-site, cross-process navigations within the same "unit of related browsing contexts" in spec terms).

That said, I'm fairly opposed to putting WebAPK parameters (even opaque ones) inside BrowsingInstance and SiteInstance directly, since that's a layering violation (and also suggests we have general support for process customization when we don't).  BrowsingInstance in particular is abstracted away even from the rest of content/, hidden entirely behind the SiteInstance API.  Its code should only care about grouping SiteInstances.

For the ComputeWebKitPrefs suggestion, I agree it's not perfect and can't prevent re-use of a customized process.  (As you mention, though, maybe punting on re-use for now is ok?)  Passing up the RVH's SiteInstance might make sense, since that gives the embedder a chance to look up whether the process needs the parameter or not based on SiteInstance and hopefully BrowsingInstance (though the latter may be slightly awkward).  Conceivably, we may be able to add some public notion of what BrowsingInstance a SiteInstance belongs to for cases like this (e.g., some kind of group ID or a "primary SiteInstance" for the BrowsingInstance like site_details.cc tracks), though probably not phrased in terms of BrowsingInstance.  That would let you set a BrowsingInstance-wide parameter on each process, at least for the time being.

I mentioned this imperfect approach because the content module simply doesn't support customizing child processes like this today.  It's a substantial project to do it correctly, including updating how process reuse works to take into account some generalizable notion of which things are allowed to share.  There's a few reasons we may want to do this anyway someday, but I don't think it's something that could be done in the short term.

Does that help clarify?

A few additional thoughts after chatting with Nick...

One alternative to exposing the BrowsingInstance is having the embedder track the relationship itself via the WebContentsDelegate or TabHelper or something like that (where it would know whether it's WebApk or a normal tab).  It's the problem the TaskManager faces in general-- keeping track of what WebContents were created for.  content/ doesn't generally try to solve that for the embedder.  That said, I'm not entirely opposed to something like a group ID for SiteInstances if needed.

Also, when we do tackle the re-use problem, it will be a generalization of what we already do for extensions (creating some extensible notion of equivalence classes that can share with each other).  It can get tricky because the classes may be based on either the type of WebContents or context (e.g., WebApk) or the origin/privilege level (e.g., extensions or web sites that need dedicated processes).  I won't go too much into design plans here, but just noted it to give a sense for why we probably shouldn't tackle it for this CL.

It's certainly worth also considering if we truly want this. The resource attribution is a neat aspect, but have we checked if it even makes a noticeable difference?

I say this because I tried to test this out, and wasn't able to get a service to register on the user-visible memory / battery use pages:
https://groups.google.com/a/google.com/d/topic/android-chatty-eng/UmO9u6yNgLo/discussion

I think it would need to be a pretty heavy webapk and also have a significant amount of usage for it to show up on the battery usage or memory usage screens (since they appear to measure over such a long time period, short usage doesn't even seem to register).

Arguably, disk and data usage are the more important attributions to get right, and this doesn't accomplish that.

I'm still thinking through what creis said, but I can respond to agrieve's comment now :p

If you are saying webapk should stop spawning renderer processes under its package, and give up resource attribution altogether, then great, let's do that. Anyone wanna make a clear decision here?

+Sam I believe was seeing some non-trivial resource usage for a PWA. I can imagine that someone doing heavy work in SW could show up (e.g. Maps?). Grace asked to continue pursuing this when the option of killing it came up 1-2 weeks ago

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

commit 1021ed20bd0720753b316270561c691e26f93cf7
Author: boliu <boliu@chromium.org>
Date: Fri Mar 03 20:13:40 2017

android: Comment to implement DidSwapBuffers for vulkan

To match GL path and implement DidSwapBuffers. Right now there isn't a
way to even build android with vulkan enabled, so not bothering to write
the code now, and just adding a TODO.

BUG= 664530 ,  582558 

Review-Url: https://codereview.chromium.org/2727583003
Cr-Commit-Position: refs/heads/master@{#454659}

[modify] https://crrev.com/1021ed20bd0720753b316270561c691e26f93cf7/content/browser/renderer_host/compositor_impl_android.cc

oops, wrong bug. #31 should have been crbug.com/512636

Alright, past busy season for me.. hopefully it's over for you too :p

Correctness: I understand that for other use cases, content has to understand when it is ok to reuse a renderer process. But as already stated above (#19/20), that's explicitly not an issue here.

Layering: I understand if BrowsingInstance and implementation details need to be hidden and isolated. But honestly I don't really see the violation of passing parameters through it to reach a lower layer. Opaqueness is "relatively" here I suppose; I could define a public struct that's then only used on android.

Looked into the suggestion of passing up SiteInstance through a ContentBrowserClient override method. I didn't find any method to look up WebContents given a SiteInstance. However embedder can definitely keep track of a map of initial SiteInstance to WebContents, and use that. Presumably any time the SiteInstance changes, then it's changing to something that's no longer under that webapk. I could look into implementing that if you think that's a good way to go?

(Removing from release block list here too.)

creis or other content owners: ping for #33 above?

Are we at all ready to move on this or are you blocked on me/others as part of your other refactors to child process launcher?

(sorry, not trying to pressure you - know I've been slow on this, just trying to get a sense of where we're at :)

waiting for creis or someone else to repsond to #33

Sorry, I think it's been blocked on me.

boliu@ wrote:
> Looked into the suggestion of passing up SiteInstance through a ContentBrowserClient
> override method. I didn't find any method to look up WebContents given a SiteInstance. 

Right, because a SiteInstance can span multiple WebContents.

> However embedder can definitely keep track of a map of initial SiteInstance to 
> WebContents, and use that. Presumably any time the SiteInstance changes, then it's 
> changing to something that's no longer under that webapk. I could look into implementing 
> that if you think that's a good way to go?

So you'd only be tracking the first SiteInstance created by the WebAPK (including popups but not any cross-process navigations within the BrowsingInstance)?  That might be easier.  Why do we need to map it to WebContents?

(I tried to figure out for myself by re-reading the bug, but no luck.)

> Why do we need to map it to WebContents?

Good question.

Originally it was because most of chrome code was operating on WebContents, so I didn't think too much about it. Thinking more, it probably should be a map<SiteInstance, LauncherParamId> instead.

But then thinking more, it's not really possible to have a map with SiteInstance as the key, since there is no way to observe its destruction, and polling HasOneRef is terrible. So how about add UserData support for SiteInstance?

UserData on SiteInstance might be something we could do.

Nick points out that SiteInstance has a bit of a hard-to-predict lifetime (since it can be kept alive by NavigationEntries in session history, etc), and that does tend to make reasoning about UserData objects a little tricky.

Nick also was wondering why WebContents UserData wouldn't be sufficient for this.  I know we shifted from WebContents to BrowsingInstance and SiteInstance to keep track of popups (etc) as part of the WebAPK as well, but there is a WebContentsObserver::DidOpenRequestedURL that you could listen to for all such cases-- you'd get a notification whenever a new WebContents was made from one that you're watching.  That would let us track the bit on WebContents itself.  We could even clear it on a cross-process navigation from an observer if you wanted to.

IIUC, the challenge there is that ComputeWebKitPrefs doesn't pass up the WebContents to ContentBrowserClient (only the RenderViewHost), and that RenderViewHostDelegate::GetAsWebContents() is off-limits?

Sigh, there's obstacles everywhere we look.  At the moment, I'm guessing a UserData on SiteInstance might be a reasonable compromise.

> IIUC, the challenge there is that ComputeWebKitPrefs doesn't pass up the WebContents to ContentBrowserClient (only the RenderViewHost), and that RenderViewHostDelegate::GetAsWebContents() is off-limits?

Not quite..? This new parameter for launching child processes, so it's not per-RenderView. The new ContentBrowserClient callback will be passed SiteInstance, so presumably it'll be called by SiteInstance(Impl) as well, when RenderProcessHostImpl is created.

RenderViewHostDelegate::GetAsWebContents isn't actually off limits. It's exposed as WebContents::FromRenderViewHost.

> This new parameter for launching child processes

This new parameter *is* for launching child processes

Oh, I thought we were talking about a ComputeWebKitPrefs approach instead.  And with WebContents::FromRenderViewHost, maybe we could pull it all off with a WebContents-based approach, as Nick was suggesting.

Then again, maybe we're far enough from the code now that another iteration on the CL might make sense, to help see what it would look like.

> Oh, I thought we were talking about a ComputeWebKitPrefs approach instead. 

Err.. I was? Specifically, I was referring to adding something like:
ContentBrowserClient::GetChildProcessLauncherParams(Foo* foo, RendererProcessLaunchParams* params)

We discussed that Foo should be SiteInstance. And SiteInstanceImpl will call GetParams before it creates RenderProcessHostImpl.


Does that not match what you had in your mind?

No, I was suggesting actually making it a WebPreference within ComputeWebKitPrefs that gets sent down to the renderer via IPC, since that's the only way that we (slightly) customize renderer processes today.  That's where the RVH/WebContents talk was coming from, since it's what ComputeWebKitPrefs operates on.  It's not pretty, but it lets us dodge the question of how future code should deal with renderer reuse.

GetChildProcessLauncherParams makes it sound like the content module supports customization of renderer processes in general, which isn't true.  Anyone else who uses this interface probably won't realize that it will interact poorly with renderer process sharing-- you might end up in an existing process where the param isn't present, and even when a new process is created with the parameter, you might end up having unrelated tabs put into that process later.

I understand that your use case is ok with ignoring that, but it would require a huge change to support that API in general, and future users of this code may not realize that.  I'm hoping to avoid a bunch of warning comments to that effect, though I realize it might be necessary.

WebPreference (or RendererPreferences) is are parameters sent to renderer after the renderer is started already. This parameter controls how the renderer process is launched, ie it's a parameter to ChildProcessLauncher, before renderer process is launched.

So.. I dunno. I see your point too about correctness in general, but I can't think of any other way around implementing this feature in webapk. Maybe make the content API android-only? yuck :/

Maybe should escalate up to grace

Making it Android-only doesn't seem necessary-- it seems like it has the same motivation as a warning comment, but other Android features might get bitten by the reuse issue.  If we add the API, I think we should just document the limitations clearly.

Grace's opinion could help, though I think jam@ should probably weigh in as well in terms of the content API and RenderProcessHost behavior.  Maybe we can come to agreement about whether renderer process customization should be supported in the future and whether a short-term API that has issues around process reuse is ok.

jam@: Can I get your thoughts on comments 44-47, and whether we should add a GetChildProcessLauncherParams API keyed off SiteInstance?  (This is about how to do resource accounting for WebContents opened within a WebAPK.)

managed to catch grace between meetings, she basically said let's do this correctly and prevent renderer reuse between chrome and webapk (and between different webapks as well)

so then.. how should this be done correctly? grace mentioned renderer reuse is already prevented for things like incognito, webui, and extension processes. I guess this should behave similarly like those things?

Glad to hear.  We'll probably want some design notes on it-- the WebUI and extension cases are handled a bit ad hoc (through bindings and custom logic in IsSuitableHost).  We'll likely want to generalize it a bit to handle cases like this.  (Incognito is a separate Profile/BrowserContext, so that's kept separate at a higher level.)

I'm tied up with another fire today, but we can talk about design tomorrow or early next week.  If you want to start looking, IsSuitableHost is probably the right starting point.

 Issue 611842  has been merged into this issue.

Bo: do you think you could pick this up in Q3?

sure will try

getting back to this, hopefully won't be derailed again..

creis: so right now, content just passes around url, assuming that's enough to decide if a given RPH is suitable. That will need to change.

So assuming we'll be creating some sort of "RPH creation params", then this will need to keep creation params along side the URL in many places it seems? Looking at caller of IsSuitableHost, that would involve SiteInstace, and NavigationController/Entry as well.

Does that sound reasonable?

Just perusing ChromeContentBrowserClient and there already seems to be so many ways of customizing SiteInstance/RPH logic... I can see the hesitation to add more but it looks like there's precedent for doing this. For example: GetOriginsRequiringDedicatedProcess is somehwaht similar in spirit. However, we're not dealing with origins and we need custom creation params. I wonder if there's a refactoring that suits are needs and can help clean this up.

To be clear: the thing we're targeting here is effetively a service-worker's scope/set of urls for an app manifest and there will be other things keyed off this same logical grouping (e.g. permission/network delegation). Granted I'm not an owner and only scanning the code, but it does seem like SiteInstances is a pretty closes abstraction. So I think the suggestion in #47 makes sense to me

> To be clear: the thing we're targeting here is effetively a service-worker's scope/set of urls for an app manifest and there will be other things keyed off this same logical grouping (e.g. permission/network delegation).

Wait.. that sounds like a different thing than the original feature discussed here. I thought this is just to make sure all the webcontents inside a webapk activity get their own creation param. Nothing to do with origins and whatnot?

I understand if webapk only wants to load a list of origins in its webcontents, and probably pop back to chrome if it ever navigates to somewhere outside that list. But that's a separate feature?

yes, you're right, it's to make webcontents inside a webapk activity to get their own creation params. but what is a webapk? the intents in to the webapk correspond to a serviceworker/manifest scope. Similarly a push notification for a service worker would want to have a similar conceptual mapping. Sorry for making things more confusing but what I was trying to get at is that this may just be the first feature which hits this use case, but as we make more pwa-y things, other features may key off this concept. 

maybe will run into that with implementation. if chrome proper and a webapk both wants to start service worker for same origin, then which creation param should that worker process get? or should just create workers and they won't be shared? or maybe chrome proper should have redirected that url to webapk in the first place so this case should never happen?

might actually want a decision now. my original plan was to key off the webapk activity. but if the api required here is actually "register creation params for origin", then that's a *very* different implementation

Comment 57: Agreed, we need to decide which problem you're trying to solve here.  Making it origin-based would mean that a tab that started in a WebAPK process might do a cross-site / cross-process navigation to a process that's back in Chrome, so our earlier discussion about BrowsingInstance would be incorrect.  Once you settle on the requirements, we can figure out the right way to handle them.

(Also, it's worth noting that SiteInstance now has a ProcessReusePolicy since the last time we discussed this.  It's used partly for ServiceWorkers, but also to try to generalize some of the SiteInstance / process logic.  Not sure how useful it is for this problem yet, but may turn out relevant.)

"Agreed, we need to decide which problem you're trying to solve here.  Making it origin-based would mean that a tab that started in a WebAPK process might do a cross-site / cross-process navigation to a process that's back in Chrome..."

but that would also cause it to leave the webapk in the ui-layer. I'm fine with doing it based on visibility but it sort of seems like a proxy for what I was suggesting. That said, content probably doesn't want to know about this regardless (is "PWA" a content concept?) so you'd be delegating out anyway.

And it's definitely not just origin based - that would be overly simplifying

+falken since this involves SW process behaviour so FYI and to keep us honest :)

it seems like SW process model is very specialized too and makes sense given the nature of it.
e.g. today if you have two tabs on same page you'll have have two different renderers and SW can be running in either process depending on last one to interact with it, whether it was killed, etc. so I think if we remote the renderer for a visible webapk to that webapk, as long as the SW follows it in that case, then that's fine. If you launch another tab in chrome, it would re-use and that's fine.

Now for the case of a push notification, we could add extra logic to also launch it it in the webapk, but that seems like a nice-to-have and not a strong requirement. The reason I'm interested in that for the long-term is that you can imagine a future world with background sync doing lots of work in the backgruond and if that's really on behalf of a webapk it should be attributed to it. In practice, that's not really an issue today.

Basically, I think we should imagine a webapk renderer like a different tab in regular tabbed chrome

obsolete - we got rid of the renderer process