Add DCHECK for shared_timer_suspended_ which shouldn't be negative

shared_timer_suspended_ in WebKitPlatformSupportImpl shouldn't be negative. 

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

[hosung, 2014-02-28 06:39:16]

PTAL

[jamesr, 2014-02-28 23:14:34]

If the browser is calling ResumeShared...() on a render process that was not
suspended, isn't that a bug on the browser side not the renderer side?

[wfa511com, 2014-03-02 12:38:29]

511
بتاريخ 2014 3 1 02:14، كتبها <jamesr@chromium.org>:

> If the browser is calling ResumeShared...() on a render process that was
> not
> suspended, isn't that a bug on the browser side not the renderer side?
>
> https://codereview.chromium.org/181823006/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hosung, 2014-03-03 07:14:21]

On 2014/02/28 23:14:34, jamesr wrote:
> If the browser is calling ResumeShared...() on a render process that was not
> suspended, isn't that a bug on the browser side not the renderer side?

I know what you say. But, if render process is killed by system, browser side
doesn't know about it right away.
There is no notification for it. So, we should add some prevent code in render
side.
Plz check the following scenario.

< step 1. normal case : multiple tabs are opened >
browser process   render processes
host ID = 1         sandbox_process1 (shared_timer_suspended_ = 0)
host ID = 2         sandbox_process2 (shared_timer_suspended_ = 0)
host ID = 3         sandbox_process3 (shared_timer_suspended_ = 0)

< step 2. suspend sharedtimer>
browser process   render processes
host ID = 1         sandbox_process1 (shared_timer_suspended_ = 1)
host ID = 2         sandbox_process2 (shared_timer_suspended_ = 1)
host ID = 3         sandbox_process3 (shared_timer_suspended_ = 1)

< step 3. sandbox_process1 is killed by system>
browser process   render processes
host ID = 1         none (sandbox_process1 is killed)
host ID = 2         sandbox_process2 (shared_timer_suspended_ = 1)
host ID = 3         sandbox_process3 (shared_timer_suspended_ = 1)

< step 4. create new render process>
browser process   render processes
host ID = 1         sandbox_process4 (shared_timer_suspended_ = 0) It is
created, so initial value of shared_timer_suspended_ is 0
host ID = 2         sandbox_process2 (shared_timer_suspended_ = 1)
host ID = 3         sandbox_process3 (shared_timer_suspended_ = 1)

< step 5. resume sharedtimer>
browser process   render processes
host ID = 1         sandbox_process4 (shared_timer_suspended_ = -1) this process
is stuck because shared_timer_suspended_ = -1
host ID = 2         sandbox_process2 (shared_timer_suspended_ = 0)
host ID = 3         sandbox_process3 (shared_timer_suspended_ = 0)

[jamesr, 2014-03-03 07:42:52]

Where would we reuse a host ID when creating a new render process? Can you
point me at that code?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hosung, 2014-03-04 07:47:33]

On 2014/03/03 07:42:52, jamesr wrote:
> Where would we reuse a host ID when creating a new render process? Can you
> point me at that code?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.


in step 2. SuspendWebKitSharedTimers() in content_view_statics.cc
suspended_processes->push_back(host->GetID()); 
// host ID are saved for suspend/resume handling.  
// The host IDs are come from g_all_hosts in render_process_host_impl.cc
// g_all_hosts stores host ID & host (RenderProcessHost* pointer)

in step 3. Even though sandbox process is killed by system, g_all_hosts is not
changed.

in step 4. Even though new render process is created, g_all_hosts is not
changed.
host ID and host are not affected. only child_process_launcher_ in
RenderProcessHostImpl is affected
new render process is created and set to child_process_launcher_ in
RenderProcessHostImpl::Init()

in step 5. we call resume sharedtimer api as below step
g_all_hosts.Get().Lookup(host_id) // host ID -> host -> child_process_launcher_
(it is new renderprocess)

What I want to say is that hostID and host pointer are not changed if the case
that render process is killed by system.

[jamesr, 2014-03-04 21:34:39]

It sounds like the bug here is on the browser process side bookkeeping and
should be fixed there.

[hosung, 2014-03-07 02:14:37]

On 2014/03/04 21:34:39, jamesr wrote:
> It sounds like the bug here is on the browser process side bookkeeping and
> should be fixed there.

PTAL

[jamesr, 2014-03-07 05:46:26]

The webkit/ change looks good but I'm not qualified to review the browser side
change. +cc pliard@ who appears to have a few TODOs in this code. Could you take
a look or suggest the right reviewer, please?

Is there any way to write a test for this.

https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
File webkit/child/webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
webkit/child/webkitplatformsupport_impl.cc:909: 
nit: DCHECK_GT() gives a slightly better error message when it fails.

[hosung, 2014-03-07 07:42:52]

On 2014/03/07 05:46:26, jamesr wrote:
> The webkit/ change looks good but I'm not qualified to review the browser side
> change. +cc pliard@ who appears to have a few TODOs in this code. Could you
take
> a look or suggest the right reviewer, please?
> 
> Is there any way to write a test for this.
> 
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> File webkit/child/webkitplatformsupport_impl.cc (right):
> 
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> webkit/child/webkitplatformsupport_impl.cc:909: 
> nit: DCHECK_GT() gives a slightly better error message when it fails.

PTAL

In case of automation test, it is difficult to make it because suspend/resume
sharedtimer should be called in app side and render process should be killed by
system side.

[Philippe, 2014-03-07 09:02:25]

On 2014/03/07 07:42:52, hosung.you wrote:
> On 2014/03/07 05:46:26, jamesr wrote:
> > The webkit/ change looks good but I'm not qualified to review the browser
side
> > change. +cc pliard@ who appears to have a few TODOs in this code. Could you
> take
> > a look or suggest the right reviewer, please?
> > 
> > Is there any way to write a test for this.
> > 
> >
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> > File webkit/child/webkitplatformsupport_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> > webkit/child/webkitplatformsupport_impl.cc:909: 
> > nit: DCHECK_GT() gives a slightly better error message when it fails.
> 
> PTAL
> 
> In case of automation test, it is difficult to make it because suspend/resume
> sharedtimer should be called in app side and render process should be killed
by
> system side.

Thanks Hosung!

As James says, it's a bug on the browser side. Yaron (yfriedman@) has a CL for
it: https://codereview.chromium.org/179123002/. The DCHECK that you are adding
is great in my opinion though.

[hosung, 2014-03-07 09:35:52]

On 2014/03/07 09:02:25, Philippe wrote:
> On 2014/03/07 07:42:52, hosung.you wrote:
> > On 2014/03/07 05:46:26, jamesr wrote:
> > > The webkit/ change looks good but I'm not qualified to review the browser
> side
> > > change. +cc pliard@ who appears to have a few TODOs in this code. Could
you
> > take
> > > a look or suggest the right reviewer, please?
> > > 
> > > Is there any way to write a test for this.
> > > 
> > >
> >
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> > > File webkit/child/webkitplatformsupport_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> > > webkit/child/webkitplatformsupport_impl.cc:909: 
> > > nit: DCHECK_GT() gives a slightly better error message when it fails.
> > 
> > PTAL
> > 
> > In case of automation test, it is difficult to make it because
suspend/resume
> > sharedtimer should be called in app side and render process should be killed
> by
> > system side.
> 
> Thanks Hosung!
> 
> As James says, it's a bug on the browser side. Yaron (yfriedman@) has a CL for
> it: https://codereview.chromium.org/179123002/. The DCHECK that you are adding
> is great in my opinion though.

ohh, there is same bug :( I didn't know that.
Then, I will check-in webkitplatformsupport_impl.cc only.

[hosung, 2014-03-07 10:09:28]

On 2014/03/07 05:46:26, jamesr wrote:
> The webkit/ change looks good but I'm not qualified to review the browser side
> change. +cc pliard@ who appears to have a few TODOs in this code. Could you
take
> a look or suggest the right reviewer, please?
> 
> Is there any way to write a test for this.
> 
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> File webkit/child/webkitplatformsupport_impl.cc (right):
> 
>
https://codereview.chromium.org/181823006/diff/10001/webkit/child/webkitplatf...
> webkit/child/webkitplatformsupport_impl.cc:909: 
> nit: DCHECK_GT() gives a slightly better error message when it fails.

PTAL

in case of content_view_statics.cc, there was same bug which are in progress of
review.
So, I want to apply webkitplatformsupport_impl.cc only.

[jamesr, 2014-03-08 01:47:33]

lgtm

[hosung, 2014-03-08 11:28:44]

The CQ bit was checked by hosung.you@samsung.com

[hosung, 2014-03-08 11:29:05]

The CQ bit was unchecked by hosung.you@samsung.com

[hosung, 2014-03-08 11:30:24]

The CQ bit was checked by hosung.you@samsung.com

[commit-bot: I haz the power, 2014-03-08 11:36:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/70001

[commit-bot: I haz the power, 2014-03-08 11:36:16]

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

[commit-bot: I haz the power, 2014-03-08 11:36:17]

Failed to apply patch for webkit/child/webkitplatformsupport_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: webkit/child/webkitplatformsupport_impl.cc
  |diff --git a/webkit/child/webkitplatformsupport_impl.cc
b/webkit/child/webkitplatformsupport_impl.cc
  |index
c958c38c039db5aa9268de4a85f45fec2309a666..3a246fa67acb3e390fec9b0d3840a76441bf90c9
100644
  |--- a/webkit/child/webkitplatformsupport_impl.cc
  |+++ b/webkit/child/webkitplatformsupport_impl.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       webkit/child/webkitplatformsupport_impl.cc
Index: webkit/child/webkitplatformsupport_impl.cc
diff --git a/webkit/child/webkitplatformsupport_impl.cc
b/webkit/child/webkitplatformsupport_impl.cc
index
c958c38c039db5aa9268de4a85f45fec2309a666..3a246fa67acb3e390fec9b0d3840a76441bf90c9
100644
--- a/webkit/child/webkitplatformsupport_impl.cc
+++ b/webkit/child/webkitplatformsupport_impl.cc
@@ -905,6 +905,8 @@ void WebKitPlatformSupportImpl::SuspendSharedTimer() {
 }
 
 void WebKitPlatformSupportImpl::ResumeSharedTimer() {
+  DCHECK_GT(shared_timer_suspended_, 0);
+
   // The shared timer may have fired or been adjusted while we were suspended.
   if (--shared_timer_suspended_ == 0 &&
       (!shared_timer_.IsRunning() ||

[hosung, 2014-03-10 05:18:43]

The CQ bit was checked by hosung.you@samsung.com

[commit-bot: I haz the power, 2014-03-10 05:18:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/90001

[hosung, 2014-03-10 05:19:04]

The CQ bit was unchecked by hosung.you@samsung.com

[hosung, 2014-03-10 05:22:36]

The CQ bit was checked by hosung.you@samsung.com

[commit-bot: I haz the power, 2014-03-10 05:22:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/90001

[commit-bot: I haz the power, 2014-03-10 06:09:53]

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

[commit-bot: I haz the power, 2014-03-10 06:09:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[hosung, 2014-03-10 06:40:44]

I got lgtm from jamesr. but the directory of the file is moved into content from
webkit. 
webkit/child/webkitplatformsupport_impl.cc ->
content/child/blink_platform_impl.cc

PTAL

[hosung, 2014-03-10 11:20:04]

adding creis@chromium.org
PTAL

[Avi (use Gerrit), 2014-03-10 12:33:47]

On 2014/03/10 06:40:44, hosung wrote:
> I got lgtm from jamesr. but the directory of the file is moved into content
from
> webkit. 
> webkit/child/webkitplatformsupport_impl.cc ->
> content/child/blink_platform_impl.cc
> 
> PTAL

Sir, you silently added three reviewers in this email step, without specifying
what you wanted.

1) Please explicitly call out who you are requesting to take action and what you
want them to do (e.g. "avi, brettw, piman, can one of you approve this?")
2) Please be patient. We are on the other side of the world.

This LGTM.

[hosung, 2014-03-10 15:06:27]

On 2014/03/10 12:33:47, Avi wrote:
> On 2014/03/10 06:40:44, hosung wrote:
> > I got lgtm from jamesr. but the directory of the file is moved into content
> from
> > webkit. 
> > webkit/child/webkitplatformsupport_impl.cc ->
> > content/child/blink_platform_impl.cc
> > 
> > PTAL
> 
> Sir, you silently added three reviewers in this email step, without specifying
> what you wanted.
> 
> 1) Please explicitly call out who you are requesting to take action and what
you
> want them to do (e.g. "avi, brettw, piman, can one of you approve this?")
> 2) Please be patient. We are on the other side of the world.
> 
> This LGTM.


Avi, Thank you very much for your advice. 
I'll keep that in mind.

[hosung, 2014-03-10 15:06:37]

The CQ bit was checked by hosung.you@samsung.com

[commit-bot: I haz the power, 2014-03-10 15:20:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/90001

[commit-bot: I haz the power, 2014-03-10 17:32:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/90001

[commit-bot: I haz the power, 2014-03-10 18:29:32]

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

[commit-bot: I haz the power, 2014-03-10 18:29:33]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[hosung, 2014-03-11 01:40:41]

Christophe, could you approve AUTHORS?
PTAL

[Inactive, 2014-03-11 01:55:31]

On 2014/03/11 01:40:41, hosung wrote:
> Christophe, could you approve AUTHORS?
> PTAL

As I said on the other CL, only a Googler can review changes to the AUTHORS file
as they need to double-check you are on the CLA.

[jamesr, 2014-03-11 20:01:50]

You're on the list, lgtm.

[hosung, 2014-03-11 21:29:55]

The CQ bit was checked by hosung.you@samsung.com

[commit-bot: I haz the power, 2014-03-11 21:53:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/110001

[commit-bot: I haz the power, 2014-03-11 23:07:14]

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

[commit-bot: I haz the power, 2014-03-11 23:07:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[hosung, 2014-03-12 00:12:40]

The CQ bit was checked by hosung.you@samsung.com

[commit-bot: I haz the power, 2014-03-12 00:13:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/130001

[commit-bot: I haz the power, 2014-03-12 05:08:54]

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

[commit-bot: I haz the power, 2014-03-12 05:08:56]

Try jobs failed on following builders: linux_chromium_rel

[hosung, 2014-03-12 05:47:56]

The CQ bit was checked by hosung.you@samsung.com

[commit-bot: I haz the power, 2014-03-12 05:48:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hosung.you@samsung.com/181823006/130001

[commit-bot: I haz the power, 2014-03-12 10:29:48]

Change committed as 256511