[DevTools] Filter any messages from previous sessions in DevToolsAgentHostImpl

[DevTools] Filter any messages from previous sessions in DevToolsAgentHostImpl

Frontend can receive response for protocol message that was sent by previous session. E.g. frontend send protocol message and then reattach then get response.
session id for procol message is added in this CL. DevToolsAgentHostImpl store current session_id, RenderFrameDevToolsAgentHost and WorkerDevToolsAgentHost add to each protocol message session id and SendResponseMessageToClient method check it after processing.

BUG=503875, 503824
TEST=run blink/tools/run_layout_tests.py inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html --repeat-each 2
R=pfeldman@chromium.org, dgozman@chromium.org, tsepez@chromium.org
TBR=jam@chromium.org

Committed: https://crrev.com/d8f30ed7ab24a5e431e51b6f9f12d76b1cb1b386
Cr-Commit-Position: refs/heads/master@{#359119}

[kozy, 2015-10-30 17:16:32]

ptal

[kozy, 2015-11-06 22:02:51]

Dmitry, please take a look.
I've added session id to protocol handler and notifications from worker.
Notifications from protocol handler is still without session_id, I think we
shouldn't provide session_id with this notification.

[dgozman, 2015-11-06 22:57:28]

Almost there!

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:171: // Filter answer for
message that was sent before reattach.
Filter any messages from previous sessions.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:172: if (session_id &&
session_id != session_id_)
Would be great to kill "zero is always valid" case.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:55: int SessionId() { return
session_id_; }
int session_id() { return session_id_; }

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.cc:102: int id =
DevToolsProtocolClient::kNoId;
nit: let's rename to command_id

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.h:18: base::Callback<void(int
session_id, const std::string& message)>;
I think it's better to replace this Notifier by Delegate with two separate
methods for command result and notification.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/forwarding_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/forwarding_agent_host.cc:21: SendMessageToClient(0,
message);
0 -> session_id()

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_client.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.cc:70: // TODO: pass
remove

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.cc:83: // TODO: !
remove

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.h:72: Response
SynthesizePinchGesture(int session_id,
Encapsulate session id into DevToolsCommandId.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:107:
std::vector<std::pair<std::string, int>> pending_messages_;
Let's comment on what all the ints mean here.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:191:
sent_messages_[call_id] = make_pair(message, session_id);
std::make_pair everywhere

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:222: for (const
std::pair<std::string, int>& message : pending_messages_)
for (const auto& pair : pending_messages_)

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:224:
std::vector<std::pair<std::string, int>> v;
Please bring back the "empty"

https://codereview.chromium.org/1408363004/diff/20001/content/common/devtools...
File content/common/devtools_messages.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/common/devtools...
content/common/devtools_messages.h:61: //   |is_last| marks the last chunk.
|call_id| and |post_state| are optional
|call_id|, |session_id| and ...

https://codereview.chromium.org/1408363004/diff/20001/content/common/devtools...
content/common/devtools_messages.h:125:
IPC_MESSAGE_ROUTED1(DevToolsHostMsg_DispatchOnInspectorBackend,
Remove it.

https://codereview.chromium.org/1408363004/diff/20001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/renderer/render...
content/renderer/render_view_browsertest.cc:303: agent()->OnAttach(host_id, 0);
0 -> 17

https://codereview.chromium.org/1408363004/diff/20001/content/renderer/render...
content/renderer/render_view_browsertest.cc:315:
agent()->OnDispatchOnInspectorBackend(0, message);
0 -> 17

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp
(right):

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp:201:
m_backendDispatcher->dispatch(0, message);
Please add a comment why zero is OK.

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp (right):

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:692:
m_notificationQueue.clear();
Let's do clear even if not attached to avoid sending outdated notifications to
next attached client.

[kozy, 2015-11-07 01:54:44]

All done. PTAL!

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:171: // Filter answer for
message that was sent before reattach.
On 2015/11/06 22:57:27, dgozman wrote:
> Filter any messages from previous sessions.

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:172: if (session_id &&
session_id != session_id_)
On 2015/11/06 22:57:27, dgozman wrote:
> Would be great to kill "zero is always valid" case.

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:55: int SessionId() { return
session_id_; }
On 2015/11/06 22:57:27, dgozman wrote:
> int session_id() { return session_id_; }

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.cc:102: int id =
DevToolsProtocolClient::kNoId;
On 2015/11/06 22:57:27, dgozman wrote:
> nit: let's rename to command_id

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.h:18: base::Callback<void(int
session_id, const std::string& message)>;
On 2015/11/06 22:57:27, dgozman wrote:
> I think it's better to replace this Notifier by Delegate with two separate
> methods for command result and notification.

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/forwarding_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/forwarding_agent_host.cc:21: SendMessageToClient(0,
message);
On 2015/11/06 22:57:27, dgozman wrote:
> 0 -> session_id()

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_client.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.cc:70: // TODO: pass
On 2015/11/06 22:57:27, dgozman wrote:
> remove

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.cc:83: // TODO: !
On 2015/11/06 22:57:27, dgozman wrote:
> remove

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.h:72: Response
SynthesizePinchGesture(int session_id,
On 2015/11/06 22:57:27, dgozman wrote:
> Encapsulate session id into DevToolsCommandId.

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:107:
std::vector<std::pair<std::string, int>> pending_messages_;
On 2015/11/06 22:57:27, dgozman wrote:
> Let's comment on what all the ints mean here.

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:191:
sent_messages_[call_id] = make_pair(message, session_id);
On 2015/11/06 22:57:27, dgozman wrote:
> std::make_pair everywhere

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:222: for (const
std::pair<std::string, int>& message : pending_messages_)
On 2015/11/06 22:57:27, dgozman wrote:
> for (const auto& pair : pending_messages_)

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:224:
std::vector<std::pair<std::string, int>> v;
On 2015/11/06 22:57:27, dgozman wrote:
> Please bring back the "empty"

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/common/devtools...
File content/common/devtools_messages.h (right):

https://codereview.chromium.org/1408363004/diff/20001/content/common/devtools...
content/common/devtools_messages.h:61: //   |is_last| marks the last chunk.
|call_id| and |post_state| are optional
On 2015/11/06 22:57:27, dgozman wrote:
> |call_id|, |session_id| and ...

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/common/devtools...
content/common/devtools_messages.h:125:
IPC_MESSAGE_ROUTED1(DevToolsHostMsg_DispatchOnInspectorBackend,
On 2015/11/06 22:57:27, dgozman wrote:
> Remove it.

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/1408363004/diff/20001/content/renderer/render...
content/renderer/render_view_browsertest.cc:303: agent()->OnAttach(host_id, 0);
On 2015/11/06 22:57:27, dgozman wrote:
> 0 -> 17

Done.

https://codereview.chromium.org/1408363004/diff/20001/content/renderer/render...
content/renderer/render_view_browsertest.cc:315:
agent()->OnDispatchOnInspectorBackend(0, message);
On 2015/11/06 22:57:28, dgozman wrote:
> 0 -> 17

Done.

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp
(right):

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp:201:
m_backendDispatcher->dispatch(0, message);
On 2015/11/06 22:57:28, dgozman wrote:
> Please add a comment why zero is OK.

Done.

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp (right):

https://codereview.chromium.org/1408363004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:692:
m_notificationQueue.clear();
On 2015/11/06 22:57:28, dgozman wrote:
> Let's do clear even if not attached to avoid sending outdated notifications to
> next attached client.

Done.

[dgozman, 2015-11-07 02:28:40]

One more step closer!

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/browser_devtools_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/browser_devtools_agent_host.cc:34: GetIOContext())),
Do not touch unnecessary lines.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:12: #include
"content/browser/devtools/devtools_protocol_delegate.h"
Let's place this to devtools/protocol.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:49: void
SendProtocolResponse(int session_id,
// DevToolsProtocolDelegate implementation.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.h (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.h:18:
DevToolsProtocolHandler(DevToolsAgentHost* agent_host,
Accept a single DevToolsAgentHostImpl* parameter.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_client.h (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.h:17:
DevToolsCommandId() : call_id(kNoId), session_id(kNoId) {}
Remove this constructor.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.h:21: class
DevToolsProtocolDelegate;
nit: empty line before this please

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.h:55: void
SendRawMessage(int session_id, const std::string& message);
Rename to SendRawNotification and remove session_id.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:146: for (const
auto& pair : old->sent_messages_)
{}

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:225:
agent_->SendMessageToClient(pair.second, pair.first);
Let's reverse the pair order.

https://codereview.chromium.org/1408363004/diff/40001/third_party/WebKit/publ...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
(right):

https://codereview.chromium.org/1408363004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:106:
virtual void sendDevToolsMessage(int sessionId, int callId, const WebString&
message, const WebString& state) {}
Bring back the space.

[kozy, 2015-11-07 03:02:35]

All done!

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/browser_devtools_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/browser_devtools_agent_host.cc:34: GetIOContext())),
On 2015/11/07 02:28:40, dgozman wrote:
> Do not touch unnecessary lines.

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:12: #include
"content/browser/devtools/devtools_protocol_delegate.h"
On 2015/11/07 02:28:40, dgozman wrote:
> Let's place this to devtools/protocol.

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.h:49: void
SendProtocolResponse(int session_id,
On 2015/11/07 02:28:40, dgozman wrote:
> // DevToolsProtocolDelegate implementation.

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.h (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.h:18:
DevToolsProtocolHandler(DevToolsAgentHost* agent_host,
On 2015/11/07 02:28:40, dgozman wrote:
> Accept a single DevToolsAgentHostImpl* parameter.

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_client.h (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.h:17:
DevToolsCommandId() : call_id(kNoId), session_id(kNoId) {}
On 2015/11/07 02:28:40, dgozman wrote:
> Remove this constructor.

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.h:21: class
DevToolsProtocolDelegate;
On 2015/11/07 02:28:40, dgozman wrote:
> nit: empty line before this please

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.h:55: void
SendRawMessage(int session_id, const std::string& message);
On 2015/11/07 02:28:40, dgozman wrote:
> Rename to SendRawNotification and remove session_id.

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:146: for (const
auto& pair : old->sent_messages_)
On 2015/11/07 02:28:40, dgozman wrote:
> {}

Done.

https://codereview.chromium.org/1408363004/diff/40001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:225:
agent_->SendMessageToClient(pair.second, pair.first);
On 2015/11/07 02:28:40, dgozman wrote:
> Let's reverse the pair order.

Done.

https://codereview.chromium.org/1408363004/diff/40001/third_party/WebKit/publ...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
(right):

https://codereview.chromium.org/1408363004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:106:
virtual void sendDevToolsMessage(int sessionId, int callId, const WebString&
message, const WebString& state) {}
On 2015/11/07 02:28:40, dgozman wrote:
> Bring back the space.

Done.

[dgozman, 2015-11-07 03:15:29]

lgtm

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.h (right):

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.h:20:
DevToolsProtocolHandler(DevToolsAgentHostImpl* agent_host);
explicit

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_client.cc (right):

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.cc:45:
notifier_->SendProtocolResponse(session_id, message);
Let's inline this.

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_delegate.h (right):

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_delegate.h:5: #ifndef
CONTENT_BROWSER_DEVTOOLS_PROTOCOL_DEVTOOLS_PROTOCOL_DELEGATE_H_
Add to gypi?

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_delegate.h:11: public:
virtual ~DevToolsProtocolDelegate() { }

[kozy, 2015-11-07 04:03:45]

Description was changed from

==========
[DevTools] Filter response for protocol msg that was sent before current attach

Frontend can receive response for protocol message that was sent by previous
session. E.g. frontend send protocol message and then reattach then get
response.
session id for procol message is added in this CL. DevToolsAgentHostImpl store
current session_id, RenderFrameDevToolsAgentHost and WorkerDevToolsAgentHost add
to each protocol message session id and SendResponseMessageToClient method check
it after processing.

BUG=503875,503824
TEST=run blink/tools/run_layout_tests.py
inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html
--repeat-each 2
R=pfeldman@chromium.org, dgozman@chromium.org
==========

to

==========
[DevTools] Filter any messages from previous sessions in DevToolsAgentHostImpl

Frontend can receive response for protocol message that was sent by previous
session. E.g. frontend send protocol message and then reattach then get
response.
session id for procol message is added in this CL. DevToolsAgentHostImpl store
current session_id, RenderFrameDevToolsAgentHost and WorkerDevToolsAgentHost add
to each protocol message session id and SendResponseMessageToClient method check
it after processing.

BUG=503875,503824
TEST=run blink/tools/run_layout_tests.py
inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html
--repeat-each 2
R=pfeldman@chromium.org, dgozman@chromium.org
==========

[kozy, 2015-11-07 04:03:57]

All done.

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_handler.h (right):

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/devtools_protocol_handler.h:20:
DevToolsProtocolHandler(DevToolsAgentHostImpl* agent_host);
On 2015/11/07 03:15:29, dgozman wrote:
> explicit

Done.

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_client.cc (right):

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_client.cc:45:
notifier_->SendProtocolResponse(session_id, message);
On 2015/11/07 03:15:29, dgozman wrote:
> Let's inline this.

Done.

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_delegate.h (right):

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_delegate.h:5: #ifndef
CONTENT_BROWSER_DEVTOOLS_PROTOCOL_DEVTOOLS_PROTOCOL_DELEGATE_H_
On 2015/11/07 03:15:29, dgozman wrote:
> Add to gypi?

Done.

https://codereview.chromium.org/1408363004/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_delegate.h:11: public:
On 2015/11/07 03:15:29, dgozman wrote:
> virtual ~DevToolsProtocolDelegate() { }

Done.

[kozy, 2015-11-07 04:04:14]

Pavel, please take a look!

[pfeldman, 2015-11-09 23:36:03]

lgtm

[kozy, 2015-11-09 23:37:03]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-09 23:39:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408363004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408363004/80001

[kozy, 2015-11-10 00:16:38]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2015-11-10 00:16:46]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2015-11-10 00:16:46]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1408363004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-11-10 00:19:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408363004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408363004/80001

[kozy, 2015-11-10 00:24:40]

kozyatinskiy@chromium.org changed reviewers:
+ dcheng@chromium.org

[kozy, 2015-11-10 00:25:36]

@dcheng, please take a look.
I need security review for content/common/devtools_messages.h

[kozy, 2015-11-10 00:28:53]

Description was changed from

==========
[DevTools] Filter any messages from previous sessions in DevToolsAgentHostImpl

Frontend can receive response for protocol message that was sent by previous
session. E.g. frontend send protocol message and then reattach then get
response.
session id for procol message is added in this CL. DevToolsAgentHostImpl store
current session_id, RenderFrameDevToolsAgentHost and WorkerDevToolsAgentHost add
to each protocol message session id and SendResponseMessageToClient method check
it after processing.

BUG=503875,503824
TEST=run blink/tools/run_layout_tests.py
inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html
--repeat-each 2
R=pfeldman@chromium.org, dgozman@chromium.org
==========

to

==========
[DevTools] Filter any messages from previous sessions in DevToolsAgentHostImpl

Frontend can receive response for protocol message that was sent by previous
session. E.g. frontend send protocol message and then reattach then get
response.
session id for procol message is added in this CL. DevToolsAgentHostImpl store
current session_id, RenderFrameDevToolsAgentHost and WorkerDevToolsAgentHost add
to each protocol message session id and SendResponseMessageToClient method check
it after processing.

BUG=503875,503824
TEST=run blink/tools/run_layout_tests.py
inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html
--repeat-each 2
R=pfeldman@chromium.org, dgozman@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
==========

[commit-bot: I haz the power, 2015-11-10 01:12:50]

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

[commit-bot: I haz the power, 2015-11-10 01:12:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kozy, 2015-11-10 22:18:49]

Description was changed from

==========
[DevTools] Filter any messages from previous sessions in DevToolsAgentHostImpl

Frontend can receive response for protocol message that was sent by previous
session. E.g. frontend send protocol message and then reattach then get
response.
session id for procol message is added in this CL. DevToolsAgentHostImpl store
current session_id, RenderFrameDevToolsAgentHost and WorkerDevToolsAgentHost add
to each protocol message session id and SendResponseMessageToClient method check
it after processing.

BUG=503875,503824
TEST=run blink/tools/run_layout_tests.py
inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html
--repeat-each 2
R=pfeldman@chromium.org, dgozman@chromium.org, dcheng@chromium.org
TBR=jam@chromium.org
==========

to

==========
[DevTools] Filter any messages from previous sessions in DevToolsAgentHostImpl

Frontend can receive response for protocol message that was sent by previous
session. E.g. frontend send protocol message and then reattach then get
response.
session id for procol message is added in this CL. DevToolsAgentHostImpl store
current session_id, RenderFrameDevToolsAgentHost and WorkerDevToolsAgentHost add
to each protocol message session id and SendResponseMessageToClient method check
it after processing.

BUG=503875,503824
TEST=run blink/tools/run_layout_tests.py
inspector/sources/debugger-breakpoints/set-conditional-breakpoint.html
--repeat-each 2
R=pfeldman@chromium.org, dgozman@chromium.org, tsepez@chromium.org
TBR=jam@chromium.org
==========

[kozy, 2015-11-10 22:18:50]

kozyatinskiy@chromium.org changed reviewers:
+ tsepez@chromium.org
- dcheng@chromium.org

[kozy, 2015-11-10 22:19:26]

@tsepez, please take a look! I neeed security review for
content/common/devtools_messages.h.

[Tom Sepez, 2015-11-10 22:29:58]

On 2015/11/10 22:19:26, kozyatinskiy wrote:
> @tsepez, please take a look! I neeed security review for
> content/common/devtools_messages.h.

Can an untrusted renderer send a bogus session id?  What happens if it does?

[kozy, 2015-11-10 22:39:24]

On 2015/11/10 22:29:58, Tom Sepez wrote:
> On 2015/11/10 22:19:26, kozyatinskiy wrote:
> > @tsepez, please take a look! I neeed security review for
> > content/common/devtools_messages.h.
> 
> Can an untrusted renderer send a bogus session id?  What happens if it does?

DevTools protocol messages with bogus session_id will be processed as before but
response will be filtered. Attach and reattach message with incorrect session_id
will make all DevTools notifications invalid and they will be filtered too. In
general in worst case we can filter DevTools protocol notifications and
responses for DevTools protocols messages which should not be filtered.

[Tom Sepez, 2015-11-10 23:20:41]

lgtm

[kozy, 2015-11-11 16:43:24]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2015-11-11 16:43:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
dgozman@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1408363004/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-11-11 16:44:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408363004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408363004/100001

[commit-bot: I haz the power, 2015-11-11 16:53:59]

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

[commit-bot: I haz the power, 2015-11-11 16:54:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kozy, 2015-11-11 16:56:54]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2015-11-11 16:56:55]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
dgozman@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1408363004/#ps120001
(title: " ")

[kozy, 2015-11-11 16:57:07]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2015-11-11 16:58:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408363004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408363004/120001

[commit-bot: I haz the power, 2015-11-11 18:20:37]

Committed patchset #6 (id:120001)

[pfeldman, 2015-11-12 04:21:44]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1437993003/ by pfeldman@chromium.org.

The reason for reverting is: Rolling this out for breaking Inspect Element..

[commit-bot: I haz the power, 2015-11-12 19:58:45]

Patchset 6 (id:??) landed as
https://crrev.com/d8f30ed7ab24a5e431e51b6f9f12d76b1cb1b386
Cr-Commit-Position: refs/heads/master@{#359119}