DevTools: do not use debug target when loading URLs for the front-end (chrome side).

DevTools: do not use debug target when loading URLs for the front-end (chrome side).

NOTRY=true

BUG=462520
Committed: https://crrev.com/175b3339e50cc84ee5ebce1350ec4f855c7c9218
Cr-Commit-Position: refs/heads/master@{#318608}

[pfeldman, 2015-02-27 10:23:06]

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

[dgozman, 2015-02-27 12:22:04]

lgtm

https://codereview.chromium.org/967513002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/967513002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:527:
net::URLFetcher::Create(gurl, net::URLFetcher::GET, this);
You should delete all pending fetchers in destructor, or they will access dead
|this|.

https://codereview.chromium.org/967513002/diff/20001/content/shell/browser/sh...
File content/shell/browser/shell_devtools_frontend.cc (right):

https://codereview.chromium.org/967513002/diff/20001/content/shell/browser/sh...
content/shell/browser/shell_devtools_frontend.cc:153:
net::URLFetcher::Create(gurl, net::URLFetcher::GET, this);
Delete pending fetchers in destructor.

[pfeldman, 2015-02-27 12:54:48]

https://codereview.chromium.org/967513002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/967513002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:527:
net::URLFetcher::Create(gurl, net::URLFetcher::GET, this);
On 2015/02/27 12:22:04, dgozman wrote:
> You should delete all pending fetchers in destructor, or they will access dead
> |this|.

Ouch. Done.

https://codereview.chromium.org/967513002/diff/20001/content/shell/browser/sh...
File content/shell/browser/shell_devtools_frontend.cc (right):

https://codereview.chromium.org/967513002/diff/20001/content/shell/browser/sh...
content/shell/browser/shell_devtools_frontend.cc:153:
net::URLFetcher::Create(gurl, net::URLFetcher::GET, this);
On 2015/02/27 12:22:04, dgozman wrote:
> Delete pending fetchers in destructor.

Done.

[pfeldman, 2015-02-27 13:16:39]

pfeldman@chromium.org changed reviewers:
+ tsepez@chromium.org

[pfeldman, 2015-02-27 13:16:40]

@tsepez: please review devtools_embedder_message_dispatcher.h. 

- There should be no changes security-wise, no new capabilities exposed
- We are now making devtools front-end use embedder to load external resources,
while we used to make inspected page do that for us which was conceptually
wrong.

Motivation: devTools can operate with no target or against remote target, so
fetching additional resources such as source maps should be done locally.

[Tom Sepez, 2015-02-27 17:25:20]

On 2015/02/27 13:16:40, pfeldman wrote:
> @tsepez: please review devtools_embedder_message_dispatcher.h. 
> 
> - There should be no changes security-wise, no new capabilities exposed
> - We are now making devtools front-end use embedder to load external
resources,
> while we used to make inspected page do that for us which was conceptually
> wrong.
Seems reasonable.  For the sake of completeness, can you give me some background
on what external resources the devtools might be requesting? Scripts? If
something "bad" unexpectedly came back, we'd have a corrupted front-end, rather
than a corrupted target, right?  This feels a little riskier, but there may be
mitigating factors that you're aware of.  Thanks.

[pfeldman, 2015-02-27 17:29:20]

No matter where something "bad" comes from (be it renderer or browser), we
process it the same way in the front-end, so nothing should change there.

Resources that we fetch so far are:
- source maps - large JSON files that contain debugging information on scripts
- timeline dumps - serialized resources gathered during performance analysis

those are not executed / interpreted anyhow, all JSON-serialized. In fact, we
seem to be in a better shape post-change since potentially compromised inspected
renderer won't have a chance to affect that data we get.

[Tom Sepez, 2015-02-27 18:00:14]

Great. LGTM. Thanks for the explanation.

[pfeldman, 2015-02-27 18:21:10]

@dgozman: as agreed offline, using stream to fetch data incrementally. 
@tsepez: no changes security-wise, using same message in the dispatcher.

[dgozman, 2015-02-27 20:23:01]

lgtm

https://codereview.chromium.org/967513002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/967513002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:580: SendMessageAck(request_id,
&response);
DevToolsAPI.streamFinish before ack?

https://codereview.chromium.org/967513002/diff/60001/content/shell/browser/sh...
File content/shell/browser/shell_devtools_frontend.cc (right):

https://codereview.chromium.org/967513002/diff/60001/content/shell/browser/sh...
content/shell/browser/shell_devtools_frontend.cc:215:
web_contents()->GetMainFrame()->ExecuteJavaScript(
Call StreamFinish.

[pfeldman, 2015-02-27 20:37:42]

https://codereview.chromium.org/967513002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/967513002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.cc:580: SendMessageAck(request_id,
&response);
Lets make client responsible for this.

https://codereview.chromium.org/967513002/diff/60001/content/shell/browser/sh...
File content/shell/browser/shell_devtools_frontend.cc (right):

https://codereview.chromium.org/967513002/diff/60001/content/shell/browser/sh...
content/shell/browser/shell_devtools_frontend.cc:215:
web_contents()->GetMainFrame()->ExecuteJavaScript(
On 2015/02/27 20:23:01, dgozman wrote:
> Call StreamFinish.

Lets make client responsible for this.

[pfeldman, 2015-02-27 20:37:46]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-02-27 20:37:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/967513002/#ps60001 (title:
"same, but uses urlfetchwriter to load incrementally.")

[commit-bot: I haz the power, 2015-02-27 20:38:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/967513002/60001

[commit-bot: I haz the power, 2015-02-27 22:52:51]

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

[commit-bot: I haz the power, 2015-02-27 22:52:52]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pfeldman, 2015-02-28 07:18:08]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-02-28 07:18:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/967513002/#ps80001 (title:
"= 0 for pure virtual")

[commit-bot: I haz the power, 2015-02-28 07:18:39]

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

[commit-bot: I haz the power, 2015-02-28 07:22:50]

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

[commit-bot: I haz the power, 2015-02-28 07:22:52]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[pfeldman, 2015-02-28 08:27:20]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-02-28 08:28:30]

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

[commit-bot: I haz the power, 2015-02-28 08:33:03]

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

[commit-bot: I haz the power, 2015-02-28 08:33:04]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[pfeldman, 2015-02-28 15:52:13]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-02-28 15:52:25]

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

[commit-bot: I haz the power, 2015-02-28 15:52:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-28 15:53:27]

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