Save running SW instance info, including its SiteInstance, into the ProcessManager.

Save running SW instance info, including its SiteInstance, into the ProcessManager.

We can use this map to drop all the process references on shutdown, in order to
fix http://crbug.com/368570.

This starts to fix an assumption in the content layer that SiteInstances outlive
their RPHs so that process permissions can be controlled through the
SiteInstance instead of by process id. However, there's still work to do before
that's completely fixed.

BUG=368570
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272142

[Jeffrey Yasskin, 2014-05-20 01:24:26]

This is an alternative to http://crrev.com/286973007 that doesn't remove the
existing-process optimization.

[dominicc (has gone to gerrit), 2014-05-20 02:01:17]

Some DBCs.

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:663:
EXPECT_LE(CountRenderProcessHosts(), 1) << "Unregistering doesn't stop the "
Isn't it expected, actual?

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:15: static bool
IncrementWorkerRefcountByPid(
The casing of Refcount is inconsistent with IncrementWorkerRefCount. Which one
is right?

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:18: if (rph &&
!rph->FastShutdownStarted()) {
Would it be more readable to make this an early return w/ false?

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:58: // Let
tests specify the returned process ID.  Note: We may need to be able
One space after .?

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.h (right):

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:53: void
ReleaseWorkerProcess(int embedded_worker_id);
The parallelism of Allocate/ReleaseWorkerProcess both taking a worker ID in the
first arg is nice. (Although the diff with the method named mumbleProcess no
longer taking a process ID but a worker ID looked a little funky at first.)

[Jeffrey Yasskin, 2014-05-20 05:20:10]

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:663:
EXPECT_LE(CountRenderProcessHosts(), 1) << "Unregistering doesn't stop the "
On 2014/05/20 02:01:18, dominicc wrote:
> Isn't it expected, actual?

For EQ and NE, yes. For the inequalities,
https://code.google.com/p/chromium/codesearch/#chromium/src/testing/gtest/inc...
just uses 'val1' and 'val2'. I can switch it if you prefer though.

I also suspect that Kinuko's CL to stop the worker when its last instance goes
away will let me change this to an EXPECT_EQ(0, ...).

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:15: static bool
IncrementWorkerRefcountByPid(
On 2014/05/20 02:01:18, dominicc wrote:
> The casing of Refcount is inconsistent with IncrementWorkerRefCount. Which one
> is right?

IncrementWorkerRefCount is pre-existing, so I've fixed this one.

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:18: if (rph &&
!rph->FastShutdownStarted()) {
On 2014/05/20 02:01:18, dominicc wrote:
> Would it be more readable to make this an early return w/ false?

It looks pretty similar.

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:58: // Let
tests specify the returned process ID.  Note: We may need to be able
On 2014/05/20 02:01:18, dominicc wrote:
> One space after .?

Done.

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.h (right):

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:53: void
ReleaseWorkerProcess(int embedded_worker_id);
On 2014/05/20 02:01:18, dominicc wrote:
> The parallelism of Allocate/ReleaseWorkerProcess both taking a worker ID in
the
> first arg is nice. (Although the diff with the method named mumbleProcess no
> longer taking a process ID but a worker ID looked a little funky at first.)

I could rename them to ...For()?

[kinuko, 2014-05-20 06:30:03]

Ok I think this intermediate approach works for me (and hopefully for the team
too). Can you make sure the test runs fine with the rebased code too? (then it
almost l-g-t-m)

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/292903002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:663:
EXPECT_LE(CountRenderProcessHosts(), 1) << "Unregistering doesn't stop the "
On 2014/05/20 05:20:10, Jeffrey Yasskin wrote:
> On 2014/05/20 02:01:18, dominicc wrote:
> > Isn't it expected, actual?
> 
> For EQ and NE, yes. For the inequalities,
>
https://code.google.com/p/chromium/codesearch/#chromium/src/testing/gtest/inc...
> just uses 'val1' and 'val2'. I can switch it if you prefer though.
> 
> I also suspect that Kinuko's CL to stop the worker when its last instance goes
> away will let me change this to an EXPECT_EQ(0, ...).

Ah yes, that might be the case.

https://codereview.chromium.org/292903002/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/292903002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:16: int
process_id) {
nit: line 15-16 would probably fit into single line

https://codereview.chromium.org/292903002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:150:
ServiceWorkerProcessManager::ProcessInfo::ProcessInfo(
nit: can these ProcessInfo definitions be placed before 
ServiceWorkerProcessManager definitions (and after static / anon-namespace
ones)?  (Just because I found it's more common)

[Jeffrey Yasskin, 2014-05-21 02:01:11]

https://codereview.chromium.org/292903002/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/292903002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:16: int
process_id) {
On 2014/05/20 06:30:03, kinuko wrote:
> nit: line 15-16 would probably fit into single line

Done.

https://codereview.chromium.org/292903002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:150:
ServiceWorkerProcessManager::ProcessInfo::ProcessInfo(
On 2014/05/20 06:30:03, kinuko wrote:
> nit: can these ProcessInfo definitions be placed before 
> ServiceWorkerProcessManager definitions (and after static / anon-namespace
> ones)?  (Just because I found it's more common)

Done.

https://codereview.chromium.org/292903002/diff/70001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/292903002/diff/70001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance_unittest.cc:154:
scoped_ptr<WebContents> web_contents1(CreateNavigatedWebContents(
Hard-coding the process ID for tests in the ProcessManager stops this test from
working with the EmbeddedWorkerTestHelper. The MockRPH system lets us create
actual tabs with which to unittest this.

[michaeln1, 2014-05-21 02:08:18]

drive-by: this looks like a nice improvement to me too

[Jeffrey Yasskin, 2014-05-22 02:48:31]

Ping. Feel free to check the commit box if this looks good.

[kinuko, 2014-05-22 04:10:49]

Sorry for delay... lgtm!

[kinuko, 2014-05-22 04:10:56]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-22 04:12:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/292903002/70001

[commit-bot: I haz the power, 2014-05-22 08:36:55]

Change committed as 272142