DevTools: provide an option to return traces as streams

DevTools: provide an option to return traces as streams

- introduce IO agent that exposes streams that may be read by the client;
- introduce DevToolsIOContext to share streams between agents;
- add returnAsStream option to Tracing.end

See also: https://codereview.chromium.org/1307863003
Depends on: https://codereview.chromium.org/1308163002
BUG=456845
Committed: https://crrev.com/5bb042e95361426f054698f875a41357288045bb
Cr-Commit-Position: refs/heads/master@{#345746}

[caseq, 2015-08-22 00:13:50]

caseq@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[pfeldman, 2015-08-24 18:17:56]

The code mostly looks good. Did you think of persisting it into a file using
File interfaces and then creating your stream on top of the file?

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
File content/browser/devtools/browser_protocol.json (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
content/browser/devtools/browser_protocol.json:91: { "name": "id", "type":
"string", "description": "Identifier of a stream to read." },
Lets introduce a handle type.

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.cc:112:
io_context_.DiscardAllStreams();
You are calling it from 2(3) places. Introduce InnerDetach() ?

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_io_context.cc (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_io_context.cc:60: void Stream::Read(off_t
position, size_t max_size, ReadCallback callback) {
DCHECK file thread, also don't call on main thread.

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/pr...
File content/browser/devtools/protocol/io_handler.cc (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/pr...
content/browser/devtools/protocol/io_handler.cc:51: if (status ==
DevToolsIOContext::Stream::StatusFailure) {
Close upon failure?

[dgozman, 2015-08-24 20:45:44]

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
File content/browser/devtools/browser_protocol.json (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
content/browser/devtools/browser_protocol.json:82: "domain": "IO",
This domain does conceptually belong to renderer protocol, but this way it's
easy (until the repo merge). Add a TODO to move it after merge?

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_agent_host_impl.h:62:
devtools::DevToolsIOContext io_context_;
Maybe make IOHandler own context?

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
File content/browser/devtools/devtools_io_context.h (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
content/browser/devtools/devtools_io_context.h:39: bool
InitOnFileThreadIdNeeded();
typo: Id instead of If

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:322:
dispatcher->SetIOHandler(io_handler_.get());
nit: let's try to be alphabetical when possible.

[caseq, 2015-08-25 01:15:28]

On 2015/08/24 18:17:56, pfeldman wrote:
> The code mostly looks good. Did you think of persisting it into a file using
> File interfaces and then creating your stream on top of the file?

Yep -- I think it's going to be roughly the same in terms of amount of code
(esp. given that FileProxy is pain to use as it forbids multiple in-flight
requests), but I think handling file writes in the stream context gives us a
better abstraction, as we don't really care what the backing storage is in the
tracing agent.

>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
> File content/browser/devtools/browser_protocol.json (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
> content/browser/devtools/browser_protocol.json:91: { "name": "id", "type":
> "string", "description": "Identifier of a stream to read." },
> Lets introduce a handle type.

Done.

>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> File content/browser/devtools/devtools_agent_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> content/browser/devtools/devtools_agent_host_impl.cc:112:
> io_context_.DiscardAllStreams();
> You are calling it from 2(3) places. Introduce InnerDetach() ?

Done.

>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> File content/browser/devtools/devtools_io_context.cc (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> content/browser/devtools/devtools_io_context.cc:60: void Stream::Read(off_t
> position, size_t max_size, ReadCallback callback) {
> DCHECK file thread, also don't call on main thread.

Ouch! Thanks for spotting, fixed!

>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/pr...
> File content/browser/devtools/protocol/io_handler.cc (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/pr...
> content/browser/devtools/protocol/io_handler.cc:51: if (status ==
> DevToolsIOContext::Stream::StatusFailure) {
> Close upon failure?

We already close the underlying file on failure, so the expensive resources are
released instantly upon fail. I think keeping the handle alive gives somewhat
more comprehensive diagnostic message, though.

[caseq, 2015-08-25 01:18:01]

On 2015/08/24 20:45:44, dgozman wrote:
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
> File content/browser/devtools/browser_protocol.json (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/br...
> content/browser/devtools/browser_protocol.json:82: "domain": "IO",
> This domain does conceptually belong to renderer protocol, but this way it's
> easy (until the repo merge). Add a TODO to move it after merge?

Moved now, as I introduced a type for StreamHandle per Pavel's request and
Tracing now depends on IO for that type definition.
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> File content/browser/devtools/devtools_agent_host_impl.h (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> content/browser/devtools/devtools_agent_host_impl.h:62:
> devtools::DevToolsIOContext io_context_;
> Maybe make IOHandler own context?

I'd rather avoid inter-agent dependencies to the extent possible, this gave us a
bit of pain on the blink side in my view.

>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> File content/browser/devtools/devtools_io_context.h (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/de...
> content/browser/devtools/devtools_io_context.h:39: bool
> InitOnFileThreadIdNeeded();
> typo: Id instead of If

Thanks for spotting, fixed!

>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/re...
> File content/browser/devtools/render_frame_devtools_agent_host.cc (right):
> 
>
https://codereview.chromium.org/1307073002/diff/1/content/browser/devtools/re...
> content/browser/devtools/render_frame_devtools_agent_host.cc:322:
> dispatcher->SetIOHandler(io_handler_.get());
> nit: let's try to be alphabetical when possible.

Ditto.

[caseq, 2015-08-25 01:24:32]

Patchset #2 (id:20001) has been deleted

[pfeldman, 2015-08-25 20:53:41]

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

https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
content/browser/devtools/devtools_agent_host_impl.cc:128:
io_context_.DiscardAllStreams();
You could kill your object here.

https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/io_handler.cc (right):

https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/io_handler.cc:41: return
Response::InvalidParams("handle");
Lets have more descriptive messages. "Stream is closed"

https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/io_handler.cc:57:
client_->SendReadResponse(command_id,
It might be that we have already disconnected.

https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/io_handler.cc:63: :
Response::InvalidParams("handle");
ditto

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

https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/io_handler.h:29: void Detached() {}
Here is where you clear client and no longer use it.

https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/io_handler.h:32: Response
Open(DevToolsCommandId command_id, const std::string& blob_url,
Why is this here?

[caseq, 2015-08-25 22:26:12]

On 2015/08/25 20:53:41, pfeldman wrote:
>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> File content/browser/devtools/devtools_agent_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> content/browser/devtools/devtools_agent_host_impl.cc:128:
> io_context_.DiscardAllStreams();
> You could kill your object here.

No, because handlers keep a reference to it that remains the same even though
clients come and go.

>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> File content/browser/devtools/protocol/io_handler.cc (right):
> 
>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/io_handler.cc:41: return
> Response::InvalidParams("handle");
> Lets have more descriptive messages. "Stream is closed"

Done (as "invalid stream handle")

> 
>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/io_handler.cc:57:
> client_->SendReadResponse(command_id,
> It might be that we have already disconnected.

Yes, but as discussed offline -- let's address this separately, as this is an
old problem already present in other agents. The worst that may happen in this
case is a client receives a message addressed to a previous client, but this
likely to be dropped due to an id mismatch.

>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/io_handler.cc:63: :
> Response::InvalidParams("handle");
> ditto

done.

>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> File content/browser/devtools/protocol/io_handler.h (right):
> 
>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/io_handler.h:29: void Detached() {}
> Here is where you clear client and no longer use it.

Dropped this altogether.

>
https://codereview.chromium.org/1307073002/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/io_handler.h:32: Response
> Open(DevToolsCommandId command_id, const std::string& blob_url,
> Why is this here?

Ouch -- nuked as well.

[pfeldman, 2015-08-26 21:10:18]

lgtm

[caseq, 2015-08-26 22:23:39]

The CQ bit was checked by caseq@chromium.org

[caseq, 2015-08-26 22:23:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1307073002/#ps80001
(title: "part 1 for landing (things that depend on new protocol.json removed)")

[commit-bot: I haz the power, 2015-08-26 22:24:55]

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

[commit-bot: I haz the power, 2015-08-26 22:59:57]

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

[commit-bot: I haz the power, 2015-08-26 22:59:58]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[caseq, 2015-08-26 23:11:06]

The CQ bit was checked by caseq@chromium.org

[caseq, 2015-08-26 23:11:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1307073002/#ps100001
(title: "work around MSVC compiler error due to missing Client definition")

[commit-bot: I haz the power, 2015-08-26 23:11:46]

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

[commit-bot: I haz the power, 2015-08-27 00:38:23]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-08-27 00:39:15]

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