DevTools: fix for open-close-open.html test

DevTools: fix for open-close-open.html test

content_shell didn't detach devtools agent when
it sent closeFrontend message to the frontend process.

BUG=231948
TEST=open-close-open.html
R=jochen@chromium.org

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

[jochen (gone - plz use gerrit), 2013-06-06 09:21:19]

lgtm

[yurys, 2013-06-06 09:28:23]

https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
File content/shell/renderer/webkit_test_runner.cc (right):

https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
content/shell/renderer/webkit_test_runner.cc:370: agent->detach();
How was it called before when front-end was closing? Don't we need to fix that
place too?

[yurys, 2013-06-06 09:35:41]

https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
File content/shell/renderer/webkit_test_runner.cc (right):

https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
content/shell/renderer/webkit_test_runner.cc:370: agent->detach();
On 2013/06/06 09:28:23, Yury Semikhatsky wrote:
> How was it called before when front-end was closing? Don't we need to fix that
> place too?

Looks like WebDevToolsAgentImpl::detach() should check if front-end is still
attached. Also it seems that there can be a race:
1. First front-end close is started
2. New front-end attached.
3. Detach event is received from the first front-end and current front-end (the
new one) is cleared
 or am I missing something?

[loislo, 2013-06-06 11:40:52]

On 2013/06/06 09:35:41, Yury Semikhatsky wrote:
>
https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
> File content/shell/renderer/webkit_test_runner.cc (right):
> 
>
https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
> content/shell/renderer/webkit_test_runner.cc:370: agent->detach();
> On 2013/06/06 09:28:23, Yury Semikhatsky wrote:
> > How was it called before when front-end was closing? Don't we need to fix
that
> > place too?
> 
> Looks like WebDevToolsAgentImpl::detach() should check if front-end is still
> attached. Also it seems that there can be a race:
> 1. First front-end close is started
> 2. New front-end attached.
> 3. Detach event is received from the first front-end and current front-end
(the
> new one) is cleared
>  or am I missing something?

We have m_attached flag in WebDevToolsAgentImpl.
It is strange but we do not check it in the detach method.
In any case it should be a separate patch because it is upstream code.

About races: I think there could be a race but I saw no real life examples or
such problem in tests.
So I'd fix that when I get a real example.

[loislo, 2013-06-10 10:04:44]

On 2013/06/06 11:40:52, loislo wrote:
> On 2013/06/06 09:35:41, Yury Semikhatsky wrote:
> >
>
https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
> > File content/shell/renderer/webkit_test_runner.cc (right):
> > 
> >
>
https://codereview.chromium.org/16268007/diff/1/content/shell/renderer/webkit...
> > content/shell/renderer/webkit_test_runner.cc:370: agent->detach();
> > On 2013/06/06 09:28:23, Yury Semikhatsky wrote:
> > > How was it called before when front-end was closing? Don't we need to fix
> that
> > > place too?
> > 
> > Looks like WebDevToolsAgentImpl::detach() should check if front-end is still
> > attached. Also it seems that there can be a race:
> > 1. First front-end close is started
> > 2. New front-end attached.
> > 3. Detach event is received from the first front-end and current front-end
> (the
> > new one) is cleared
> >  or am I missing something?
> 
> We have m_attached flag in WebDevToolsAgentImpl.
> It is strange but we do not check it in the detach method.
> In any case it should be a separate patch because it is upstream code.
> 
> About races: I think there could be a race but I saw no real life examples or
> such problem in tests.
> So I'd fix that when I get a real example.

Looks like we already have a guard for this case.
DevToolsManager keeps two maps, agent2client and client2agent.
And if a new client is going to be attached then the manager unregisters and
detaches the first client.
https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/shel...

[loislo, 2013-06-10 11:02:08]

Committed patchset #1 manually as r205185 (presubmit successful).