Fixed not working inspector in case of Remote Target

Fixed not working inspector in case of Remote Target

devtools_frontend_url is not reachable when using TCPDeviceProvicer.
Fix devtools_frontend_url when it is not valid.

BUG=674394

[wanchang, 2016-12-15 04:46:51]

Description was changed from

==========
Fixed not working inspector in case of Remote Target

devtools_frontend_url is not reachable when using TCPDeviceProvicer.
Fix devtools_frontend_url when it is not valid.

BUG=674394
==========

to

==========
Fixed not working inspector in case of Remote Target

devtools_frontend_url is not reachable when using TCPDeviceProvicer.
Fix devtools_frontend_url when it is not valid.

BUG=674394
==========

[wanchang, 2016-12-15 04:46:51]

wanchang.ryu@lge.com changed reviewers:
+ dgozman@chromium.org

[wanchang, 2016-12-15 05:25:11]

The CQ bit was checked by wanchang.ryu@lge.com

[commit-bot: I haz the power, 2016-12-15 05:25:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 05:25:32]

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

[commit-bot: I haz the power, 2016-12-15 05:25:33]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[wanchang, 2016-12-15 05:27:12]

The CQ bit was unchecked by wanchang.ryu@lge.com

[wanchang, 2016-12-15 05:35:39]

Dear all,

I would like you to review the patch.

Inspector is not opened on the chrome:inspect tab, when network target is used.
Network target itself opens remote debugging port and provides invalid
devtools_frontend_url.
This patch fix the devtools_frontend_url when it is invalid.

Any comment will be appreciated.

Thanks.
Wanchang.

[wanchang, 2016-12-22 00:22:52]

Dear dgozman,

Would you review this patch  ?

Thanks
Wanchang.

[wanchang, 2016-12-22 01:49:08]

wanchang.ryu@lge.com changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-12-22 17:40:36]

I'd have to look into it to say how it should be fixed, but we've lost the
remote front-end url somewhere, so we open bundled front-end instead of the
remote one.

[wanchang, 2016-12-23 00:41:59]

On 2016/12/22 17:40:36, pfeldman wrote:
> I'd have to look into it to say how it should be fixed, but we've lost the
> remote front-end url somewhere, so we open bundled front-end instead of the
> remote one.

Dear pfeldman,

Thanks for your comment. I'm happy to start discussing with you.

I'd like share my debugging information regarding this patch. Could you take a
look this ? The front-end url is fine when the remote target is android device.
But The front-end url of others is broken.

Front-end url is set in remote debugging server

Android Browser
https://cs.chromium.org/chromium/src/chrome/browser/android/devtools_server.c...
const char kFrontEndURL[] =
    "http://chrome-devtools-frontend.appspot.com/serve_rev/%s/inspector.html";
  DevToolsAgentHost::StartRemoteDebuggingServer(
      std::move(factory),
      base::StringPrintf(kFrontEndURL, content::GetWebKitRevision().c_str()),
      base::FilePath(), base::FilePath(),
      version_info::GetProductNameAndVersionForUserAgent(), ::GetUserAgent());

Android Webview
https://cs.chromium.org/chromium/src/android_webview/native/aw_devtools_serve...
const char kFrontEndURL[] =
    "http://chrome-devtools-frontend.appspot.com/serve_rev/%s/inspector.html";
  DevToolsAgentHost::StartRemoteDebuggingServer(
      std::move(factory),
      base::StringPrintf(kFrontEndURL, content::GetWebKitRevision().c_str()),
      base::FilePath(), base::FilePath(),
      GetProduct(), GetUserAgent());

Chrome Browser
https://cs.chromium.org/chromium/src/chrome/browser/devtools/remote_debugging...
  content::DevToolsAgentHost::StartRemoteDebuggingServer(
      base::MakeUnique<TCPServerSocketFactory>(ip, port),
      std::string(),
      output_dir,
      debug_frontend_dir,
      version_info::GetProductNameAndVersionForUserAgent(),
      ::GetUserAgent());

In Chrome Browser, It uses empty string for the front-end url then, it is set by
"/devtools/inspector.html" in
https://cs.chromium.org/chromium/src/content/browser/devtools/devtools_http_h...

DevToolsHttpHandler::DevToolsHttpHandler(
    DevToolsManagerDelegate* delegate,
    std::unique_ptr<DevToolsSocketFactory> socket_factory,
    const std::string& frontend_url,
    const base::FilePath& output_directory,
    const base::FilePath& debug_frontend_dir,
    const std::string& product_name,
    const std::string& user_agent)
    : thread_(nullptr),
      frontend_url_(frontend_url),
      product_name_(product_name),
      user_agent_(user_agent),
      server_wrapper_(nullptr),
      delegate_(delegate),
      socket_factory_(nullptr),
      weak_factory_(this) {
  bool bundles_resources = frontend_url_.empty();
  if (frontend_url_.empty())
    frontend_url_ = "/devtools/inspector.html";

So, Front-end url is broken in remote target of chrome browser. As you said,
when it is broken, the bundled front-end is used. I think it is only when
DEBUG_DEVTOOLS is defined.

https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/devtools_ui.cc?r...

#if BUILDFLAG(DEBUG_DEVTOOLS)
// Local frontend url provided by InspectUI.
const char kFallbackFrontendURL[] =
    "chrome-devtools://devtools/bundled/inspector.html";
#else
// URL causing the DevTools window to display a plain text warning.
const char kFallbackFrontendURL[] =
    "data:text/plain,Cannot load DevTools frontend from an untrusted origin";
#endif  // BUILDFLAG(DEBUG_DEVTOOLS)

In my patch, it uses webkit revision of remote target to make front end url when
it is broken. I think it would be better it is possible to consider the revision
of webkit of remote target instead just using bundle front-end url.

Thanks

[eostroukhov, 2017-01-03 22:08:24]

On 2016/12/23 00:41:59, wanchang wrote:
> On 2016/12/22 17:40:36, pfeldman wrote:
> > I'd have to look into it to say how it should be fixed, but we've lost the
> > remote front-end url somewhere, so we open bundled front-end instead of the
> > remote one.
> 
> Dear pfeldman,
> 
> Thanks for your comment. I'm happy to start discussing with you.
> 
> I'd like share my debugging information regarding this patch. Could you take a
> look this ? The front-end url is fine when the remote target is android
device.
> But The front-end url of others is broken.
> 
> Front-end url is set in remote debugging server
> 
> Android Browser
>
https://cs.chromium.org/chromium/src/chrome/browser/android/devtools_server.c...
> const char kFrontEndURL[] =
>     "http://chrome-devtools-frontend.appspot.com/serve_rev/%s/inspector.html";
>   DevToolsAgentHost::StartRemoteDebuggingServer(
>       std::move(factory),
>       base::StringPrintf(kFrontEndURL, content::GetWebKitRevision().c_str()),
>       base::FilePath(), base::FilePath(),
>       version_info::GetProductNameAndVersionForUserAgent(), ::GetUserAgent());
> 
> Android Webview
>
https://cs.chromium.org/chromium/src/android_webview/native/aw_devtools_serve...
> const char kFrontEndURL[] =
>     "http://chrome-devtools-frontend.appspot.com/serve_rev/%s/inspector.html";
>   DevToolsAgentHost::StartRemoteDebuggingServer(
>       std::move(factory),
>       base::StringPrintf(kFrontEndURL, content::GetWebKitRevision().c_str()),
>       base::FilePath(), base::FilePath(),
>       GetProduct(), GetUserAgent());
> 
> Chrome Browser
>
https://cs.chromium.org/chromium/src/chrome/browser/devtools/remote_debugging...
>   content::DevToolsAgentHost::StartRemoteDebuggingServer(
>       base::MakeUnique<TCPServerSocketFactory>(ip, port),
>       std::string(),
>       output_dir,
>       debug_frontend_dir,
>       version_info::GetProductNameAndVersionForUserAgent(),
>       ::GetUserAgent());
> 
> In Chrome Browser, It uses empty string for the front-end url then, it is set
by
> "/devtools/inspector.html" in
>
https://cs.chromium.org/chromium/src/content/browser/devtools/devtools_http_h...
> 
> DevToolsHttpHandler::DevToolsHttpHandler(
>     DevToolsManagerDelegate* delegate,
>     std::unique_ptr<DevToolsSocketFactory> socket_factory,
>     const std::string& frontend_url,
>     const base::FilePath& output_directory,
>     const base::FilePath& debug_frontend_dir,
>     const std::string& product_name,
>     const std::string& user_agent)
>     : thread_(nullptr),
>       frontend_url_(frontend_url),
>       product_name_(product_name),
>       user_agent_(user_agent),
>       server_wrapper_(nullptr),
>       delegate_(delegate),
>       socket_factory_(nullptr),
>       weak_factory_(this) {
>   bool bundles_resources = frontend_url_.empty();
>   if (frontend_url_.empty())
>     frontend_url_ = "/devtools/inspector.html";
> 
> So, Front-end url is broken in remote target of chrome browser. As you said,
> when it is broken, the bundled front-end is used. I think it is only when
> DEBUG_DEVTOOLS is defined.
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/devtools_ui.cc?r...
> 
> #if BUILDFLAG(DEBUG_DEVTOOLS)
> // Local frontend url provided by InspectUI.
> const char kFallbackFrontendURL[] =
>     "chrome-devtools://devtools/bundled/inspector.html";
> #else
> // URL causing the DevTools window to display a plain text warning.
> const char kFallbackFrontendURL[] =
>     "data:text/plain,Cannot load DevTools frontend from an untrusted origin";
> #endif  // BUILDFLAG(DEBUG_DEVTOOLS)
> 
> In my patch, it uses webkit revision of remote target to make front end url
when
> it is broken. I think it would be better it is possible to consider the
revision
> of webkit of remote target instead just using bundle front-end url.
> 
> Thanks

Thanks for your analysis.

Could it be that the issue is with DevToolsHttpHandler? Would it be possible to
include the proper URL in the response?

[wanchang, 2017-01-04 01:48:31]

On 2017/01/03 22:08:24, eostroukhov wrote:
> On 2016/12/23 00:41:59, wanchang wrote:
> > On 2016/12/22 17:40:36, pfeldman wrote:
> > > I'd have to look into it to say how it should be fixed, but we've lost the
> > > remote front-end url somewhere, so we open bundled front-end instead of
the
> > > remote one.
> > 
> > Dear pfeldman,
> > 
> > Thanks for your comment. I'm happy to start discussing with you.
> > 
> > I'd like share my debugging information regarding this patch. Could you take
a
> > look this ? The front-end url is fine when the remote target is android
> device.
> > But The front-end url of others is broken.
> > 
> > Front-end url is set in remote debugging server
> > 
> > Android Browser
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/android/devtools_server.c...
> > const char kFrontEndURL[] =
> >    
"http://chrome-devtools-frontend.appspot.com/serve_rev/%s/inspector.html";
> >   DevToolsAgentHost::StartRemoteDebuggingServer(
> >       std::move(factory),
> >       base::StringPrintf(kFrontEndURL,
content::GetWebKitRevision().c_str()),
> >       base::FilePath(), base::FilePath(),
> >       version_info::GetProductNameAndVersionForUserAgent(),
::GetUserAgent());
> > 
> > Android Webview
> >
>
https://cs.chromium.org/chromium/src/android_webview/native/aw_devtools_serve...
> > const char kFrontEndURL[] =
> >    
"http://chrome-devtools-frontend.appspot.com/serve_rev/%s/inspector.html";
> >   DevToolsAgentHost::StartRemoteDebuggingServer(
> >       std::move(factory),
> >       base::StringPrintf(kFrontEndURL,
content::GetWebKitRevision().c_str()),
> >       base::FilePath(), base::FilePath(),
> >       GetProduct(), GetUserAgent());
> > 
> > Chrome Browser
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/devtools/remote_debugging...
> >   content::DevToolsAgentHost::StartRemoteDebuggingServer(
> >       base::MakeUnique<TCPServerSocketFactory>(ip, port),
> >       std::string(),
> >       output_dir,
> >       debug_frontend_dir,
> >       version_info::GetProductNameAndVersionForUserAgent(),
> >       ::GetUserAgent());
> > 
> > In Chrome Browser, It uses empty string for the front-end url then, it is
set
> by
> > "/devtools/inspector.html" in
> >
>
https://cs.chromium.org/chromium/src/content/browser/devtools/devtools_http_h...
> > 
> > DevToolsHttpHandler::DevToolsHttpHandler(
> >     DevToolsManagerDelegate* delegate,
> >     std::unique_ptr<DevToolsSocketFactory> socket_factory,
> >     const std::string& frontend_url,
> >     const base::FilePath& output_directory,
> >     const base::FilePath& debug_frontend_dir,
> >     const std::string& product_name,
> >     const std::string& user_agent)
> >     : thread_(nullptr),
> >       frontend_url_(frontend_url),
> >       product_name_(product_name),
> >       user_agent_(user_agent),
> >       server_wrapper_(nullptr),
> >       delegate_(delegate),
> >       socket_factory_(nullptr),
> >       weak_factory_(this) {
> >   bool bundles_resources = frontend_url_.empty();
> >   if (frontend_url_.empty())
> >     frontend_url_ = "/devtools/inspector.html";
> > 
> > So, Front-end url is broken in remote target of chrome browser. As you said,
> > when it is broken, the bundled front-end is used. I think it is only when
> > DEBUG_DEVTOOLS is defined.
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/devtools_ui.cc?r...
> > 
> > #if BUILDFLAG(DEBUG_DEVTOOLS)
> > // Local frontend url provided by InspectUI.
> > const char kFallbackFrontendURL[] =
> >     "chrome-devtools://devtools/bundled/inspector.html";
> > #else
> > // URL causing the DevTools window to display a plain text warning.
> > const char kFallbackFrontendURL[] =
> >     "data:text/plain,Cannot load DevTools frontend from an untrusted
origin";
> > #endif  // BUILDFLAG(DEBUG_DEVTOOLS)
> > 
> > In my patch, it uses webkit revision of remote target to make front end url
> when
> > it is broken. I think it would be better it is possible to consider the
> revision
> > of webkit of remote target instead just using bundle front-end url.
> > 
> > Thanks
> 
> Thanks for your analysis.
> 
> Could it be that the issue is with DevToolsHttpHandler? Would it be possible
to
> include the proper URL in the response?

Thanks eostroukhov for your comment.

As I understand, the frontendurl in the response of "/json/list" is used in two
cases. One case is when connecting remote debugging port directly.
https://cs.chromium.org/chromium/src/chrome/browser/devtools/frontend/devtool...
In this case, the baseurl is used with frontendurl in the client browser side if
it is a relative url.

Th other case is from chrome:inspect url.
https://cs.chromium.org/chromium/src/chrome/browser/devtools/device/devtools_...

In this case, frontendurl is used as it is to open external inspector.
https://cs.chromium.org/chromium/src/chrome/browser/devtools/devtools_window....

To make absolute url path of frontendurl in DevToolsHttpHandler, there were two
possible.
 1. local ip and port of remote debugging device
 2. proxy frontendurl like starting with
http://chrome-devtools-frontend.appspot.com/

It is not possible to connect remote debugging port with solution 1 if remote
target is behind proxy or NAT.
With solution 2, It could solve a problem of behind NAT but it is connecting the
proxy frontendurl instead the remote debugging port (behavior will be changed
from previous.)

So I think, It is better just using the frontendurl as current and get it fixed
in devtools_device_discovery side if the frontendurl is invalid.

How do you think ?

Thanks.

[lukyanov811, 2017-01-06 21:52:09]

lukyanov811@gmail.com changed reviewers:
+ lukyanov811@gmail.com

[lukyanov811, 2017-01-06 21:52:10]

lgtm

djendrius@inbox.ru

[lukyanov811, 2017-01-06 22:22:41]

lgtm

[wanchang, 2017-01-09 00:12:39]

The CQ bit was checked by wanchang.ryu@lge.com

[commit-bot: I haz the power, 2017-01-09 00:12:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 00:12:48]

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

[commit-bot: I haz the power, 2017-01-09 00:12:49]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[wanchang, 2017-01-10 00:19:54]

Could you take a look my investigation and review the patch ?