Disable the hang dialog detector when showModalDialog is running.

Disable the hang dialog detector when showModalDialog is running.

BUG=122153
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136117

[jam, 2012-05-04 21:39:53]

actually, hold on a second on reviewing this. I had a partial revert of
another change and I just tested without it and it looks like it affects it.

On Fri, May 4, 2012 at 2:38 PM, <jam@chromium.org> wrote:

> Reviewers: darin,
>
> Description:
> Disable the hang dialog detector when showModalDialog is running.
>
> BUG=122153
>
> Please review this at
http://codereview.chromium.**org/10377017/<http://codereview.chromium.org/103...
>
> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>
> Affected files:
>  M     content/browser/renderer_host/**render_view_host_impl.h
>  M     content/browser/renderer_host/**render_view_host_impl.cc
>  M     content/common/view_messages.h
>  M     content/renderer/render_view_**impl.cc
>
>
> Index: content/browser/renderer_host/**render_view_host_impl.cc
> ==============================**==============================**=======
> --- content/browser/renderer_host/**render_view_host_impl.cc
>  (revision 135224)
> +++ content/browser/renderer_host/**render_view_host_impl.cc
>  (working copy)
> @@ -148,6 +148,7 @@
>       suspended_nav_message_(NULL),
>       is_swapped_out_(swapped_out),
>       run_modal_reply_msg_(NULL),
> +      run_modal_opener_id_(MSG_**ROUTING_NONE),
>       is_waiting_for_beforeunload_**ack_(false),
>       is_waiting_for_unload_ack_(**false),
>       has_timed_out_on_unload_(**false),
> @@ -990,6 +991,13 @@
>   if (run_modal_reply_msg_) {
>     Send(run_modal_reply_msg_);
>     run_modal_reply_msg_ = NULL;
> +    RenderViewHostImpl* opener =
> +        RenderViewHostImpl::FromID(**GetProcess()->GetID(),
> run_modal_opener_id_);
> +    if (opener) {
> +      opener->**StartHangMonitorTimeout(**TimeDelta::FromMilliseconds(
> +          hung_renderer_delay_ms_));
> +    }
> +    run_modal_opener_id_ = MSG_ROUTING_NONE;
>   }
>
>   RenderWidgetHostImpl::**Shutdown();
> @@ -1053,12 +1061,17 @@
>   }
>  }
>
> -void RenderViewHostImpl::**OnMsgRunModal(IPC::Message* reply_msg) {
> +void RenderViewHostImpl::**OnMsgRunModal(int opener_id, IPC::Message*
> reply_msg) {
>   DCHECK(!run_modal_reply_msg_);
>   run_modal_reply_msg_ = reply_msg;
> +  run_modal_opener_id_ = opener_id;
>
>   content::RecordAction(**UserMetricsAction("**ShowModalDialog"));
>
> +  RenderViewHostImpl* opener =
> +      RenderViewHostImpl::FromID(**GetProcess()->GetID(),
> run_modal_opener_id_);
> +  opener->**StopHangMonitorTimeout();
> +
>   // TODO(darin): Bug 1107929: Need to inform our delegate to show this
> view in
>   // an app-modal fashion.
>  }
> Index: content/browser/renderer_host/**render_view_host_impl.h
> ==============================**==============================**=======
> --- content/browser/renderer_host/**render_view_host_impl.h
> (revision 135224)
> +++ content/browser/renderer_host/**render_view_host_impl.h
> (working copy)
> @@ -438,7 +438,7 @@
>                      bool user_gesture);
>   void OnMsgShowWidget(int route_id, const gfx::Rect& initial_pos);
>   void OnMsgShowFullscreenWidget(int route_id);
> -  void OnMsgRunModal(IPC::Message* reply_msg);
> +  void OnMsgRunModal(int opener_id, IPC::Message* reply_msg);
>   void OnMsgRenderViewReady();
>   void OnMsgRenderViewGone(int status, int error_code);
>   void OnMsgNavigate(const IPC::Message& msg);
> @@ -577,6 +577,8 @@
>   // If we were asked to RunModal, then this will hold the reply_msg that
> we
>   // must return to the renderer to unblock it.
>   IPC::Message* run_modal_reply_msg_;
> +  // This will hold the routing id of the RenderView that opened us.
> +  int run_modal_opener_id_;
>
>   // Set to true when there is a pending ViewMsg_ShouldClose message.  This
>   // ensures we don't spam the renderer with multiple beforeunload
> requests.
> Index: content/common/view_messages.h
> ==============================**==============================**=======
> --- content/common/view_messages.h      (revision 135224)
> +++ content/common/view_messages.h      (working copy)
> @@ -1257,7 +1257,8 @@
>
>  // This message is sent after ViewHostMsg_ShowView to cause the RenderView
>  // to run in a modal fashion until it is closed.
> -IPC_SYNC_MESSAGE_ROUTED0_0(**ViewHostMsg_RunModal)
> +IPC_SYNC_MESSAGE_ROUTED1_0(**ViewHostMsg_RunModal,
> +                           int /* opener_id */)
>
>  // Indicates the renderer is ready in response to a ViewMsg_New or
>  // a ViewMsg_CreatingNew_ACK.
> Index: content/renderer/render_view_**impl.cc
> ==============================**==============================**=======
> --- content/renderer/render_view_**impl.cc        (revision 135224)
> +++ content/renderer/render_view_**impl.cc        (working copy)
> @@ -2094,7 +2094,8 @@
>   if (RenderThreadImpl::current())  // Will be NULL during unit tests.
>     RenderThreadImpl::current()->**DoNotSuspendWebKitSharedTimer(**);
>
> -  SendAndRunNestedMessageLoop(**new ViewHostMsg_RunModal(routing_**id_));
> +  SendAndRunNestedMessageLoop(**new ViewHostMsg_RunModal(
> +      routing_id_, opener_id_));
>  }
>
>  bool RenderViewImpl::**enterFullScreen() {
>
>
>

[jam, 2012-05-04 21:43:41]

ok I've localized which line from the other change also affects this, I'll
send an email about it on our thread. but for now, this is good to review.

On Fri, May 4, 2012 at 2:39 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> actually, hold on a second on reviewing this. I had a partial revert of
> another change and I just tested without it and it looks like it affects it.
>
>
> On Fri, May 4, 2012 at 2:38 PM, <jam@chromium.org> wrote:
>
>> Reviewers: darin,
>>
>> Description:
>> Disable the hang dialog detector when showModalDialog is running.
>>
>> BUG=122153
>>
>> Please review this at
http://codereview.chromium.**org/10377017/<http://codereview.chromium.org/103...
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/**src/
>>
>> Affected files:
>>  M     content/browser/renderer_host/**render_view_host_impl.h
>>  M     content/browser/renderer_host/**render_view_host_impl.cc
>>  M     content/common/view_messages.h
>>  M     content/renderer/render_view_**impl.cc
>>
>>
>> Index: content/browser/renderer_host/**render_view_host_impl.cc
>> ==============================**==============================**=======
>> --- content/browser/renderer_host/**render_view_host_impl.cc
>>  (revision 135224)
>> +++ content/browser/renderer_host/**render_view_host_impl.cc
>>  (working copy)
>> @@ -148,6 +148,7 @@
>>       suspended_nav_message_(NULL),
>>       is_swapped_out_(swapped_out),
>>       run_modal_reply_msg_(NULL),
>> +      run_modal_opener_id_(MSG_**ROUTING_NONE),
>>       is_waiting_for_beforeunload_**ack_(false),
>>       is_waiting_for_unload_ack_(**false),
>>       has_timed_out_on_unload_(**false),
>> @@ -990,6 +991,13 @@
>>   if (run_modal_reply_msg_) {
>>     Send(run_modal_reply_msg_);
>>     run_modal_reply_msg_ = NULL;
>> +    RenderViewHostImpl* opener =
>> +        RenderViewHostImpl::FromID(**GetProcess()->GetID(),
>> run_modal_opener_id_);
>> +    if (opener) {
>> +      opener->**StartHangMonitorTimeout(**TimeDelta::FromMilliseconds(
>> +          hung_renderer_delay_ms_));
>> +    }
>> +    run_modal_opener_id_ = MSG_ROUTING_NONE;
>>   }
>>
>>   RenderWidgetHostImpl::**Shutdown();
>> @@ -1053,12 +1061,17 @@
>>   }
>>  }
>>
>> -void RenderViewHostImpl::**OnMsgRunModal(IPC::Message* reply_msg) {
>> +void RenderViewHostImpl::**OnMsgRunModal(int opener_id, IPC::Message*
>> reply_msg) {
>>   DCHECK(!run_modal_reply_msg_);
>>   run_modal_reply_msg_ = reply_msg;
>> +  run_modal_opener_id_ = opener_id;
>>
>>   content::RecordAction(**UserMetricsAction("**ShowModalDialog"));
>>
>> +  RenderViewHostImpl* opener =
>> +      RenderViewHostImpl::FromID(**GetProcess()->GetID(),
>> run_modal_opener_id_);
>> +  opener->**StopHangMonitorTimeout();
>> +
>>   // TODO(darin): Bug 1107929: Need to inform our delegate to show this
>> view in
>>   // an app-modal fashion.
>>  }
>> Index: content/browser/renderer_host/**render_view_host_impl.h
>> ==============================**==============================**=======
>> --- content/browser/renderer_host/**render_view_host_impl.h
>> (revision 135224)
>> +++ content/browser/renderer_host/**render_view_host_impl.h
>> (working copy)
>> @@ -438,7 +438,7 @@
>>                      bool user_gesture);
>>   void OnMsgShowWidget(int route_id, const gfx::Rect& initial_pos);
>>   void OnMsgShowFullscreenWidget(int route_id);
>> -  void OnMsgRunModal(IPC::Message* reply_msg);
>> +  void OnMsgRunModal(int opener_id, IPC::Message* reply_msg);
>>   void OnMsgRenderViewReady();
>>   void OnMsgRenderViewGone(int status, int error_code);
>>   void OnMsgNavigate(const IPC::Message& msg);
>> @@ -577,6 +577,8 @@
>>   // If we were asked to RunModal, then this will hold the reply_msg that
>> we
>>   // must return to the renderer to unblock it.
>>   IPC::Message* run_modal_reply_msg_;
>> +  // This will hold the routing id of the RenderView that opened us.
>> +  int run_modal_opener_id_;
>>
>>   // Set to true when there is a pending ViewMsg_ShouldClose message.
>>  This
>>   // ensures we don't spam the renderer with multiple beforeunload
>> requests.
>> Index: content/common/view_messages.h
>> ==============================**==============================**=======
>> --- content/common/view_messages.h      (revision 135224)
>> +++ content/common/view_messages.h      (working copy)
>> @@ -1257,7 +1257,8 @@
>>
>>  // This message is sent after ViewHostMsg_ShowView to cause the
>> RenderView
>>  // to run in a modal fashion until it is closed.
>> -IPC_SYNC_MESSAGE_ROUTED0_0(**ViewHostMsg_RunModal)
>> +IPC_SYNC_MESSAGE_ROUTED1_0(**ViewHostMsg_RunModal,
>> +                           int /* opener_id */)
>>
>>  // Indicates the renderer is ready in response to a ViewMsg_New or
>>  // a ViewMsg_CreatingNew_ACK.
>> Index: content/renderer/render_view_**impl.cc
>> ==============================**==============================**=======
>> --- content/renderer/render_view_**impl.cc        (revision 135224)
>> +++ content/renderer/render_view_**impl.cc        (working copy)
>> @@ -2094,7 +2094,8 @@
>>   if (RenderThreadImpl::current())  // Will be NULL during unit tests.
>>     RenderThreadImpl::current()->**DoNotSuspendWebKitSharedTimer(**);
>>
>> -  SendAndRunNestedMessageLoop(**new ViewHostMsg_RunModal(routing_**
>> id_));
>> +  SendAndRunNestedMessageLoop(**new ViewHostMsg_RunModal(
>> +      routing_id_, opener_id_));
>>  }
>>
>>  bool RenderViewImpl::**enterFullScreen() {
>>
>>
>>
>

[darin (slow to review), 2012-05-04 21:50:19]

LGTM

[jam, 2012-05-05 00:28:51]

Darin: wanna take another quick look?

[darin (slow to review), 2012-05-07 18:11:02]

LGTM

[commit-bot: I haz the power, 2012-05-07 21:53:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-08 00:34:56]

Try job failure for 10377017-2004 (retry) on win_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-08 02:01:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-08 06:36:31]

Try job failure for 10377017-2004 (retry) (retry) on win_rel for step
"browser_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-08 20:05:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-08 21:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-08 21:35:38]

Try job failure for 10377017-2004 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-08 23:25:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-09 00:01:25]

Try job failure for 10377017-2004 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-09 00:53:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-09 03:46:20]

Try job failure for 10377017-2004 (retry) on win_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-09 04:27:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-09 08:30:15]

Try job failure for 10377017-2004 (retry) (retry) on win_rel for step
"browser_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-09 16:02:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-09 16:23:56]

Try job failure for 10377017-2004 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-09 18:45:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jam@chromium.org/10377017/2004

[commit-bot: I haz the power, 2012-05-09 20:38:46]

Change committed as 136117