Emit an error log line when attempting to connect to an app at an invalid URL.

Emit an error log line when attempting to connect to an app at an invalid URL.

BUG=431955
R=aa@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/22190df420651906c772fe7bda993809d58bc1fd

[azani, 2014-11-18 23:02:12]

azani@chromium.org changed reviewers:
+ aa@chromium.org, jamesr@chromium.org

[azani, 2014-11-18 23:02:12]

Not sure if this is the right approach. It does mean a non-cryptic error message
will be logged which was the bug's complaint, but I'm also not convinced this is
the right place to check that the URL is valid.

[Aaron Boodman, 2014-11-19 20:29:28]

https://codereview.chromium.org/735863002/diff/20001/mojo/application_manager...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/735863002/diff/20001/mojo/application_manager...
mojo/application_manager/application_manager.cc:101: GURL
app_gurl(app_url.get());
If we were going to check the URL before requesting it, then I think it should
be in ApplicationManager::ConnectToApplication() instead because that's a better
chokepoint.

But I think we should probably not check the URL before requesting it, and just
use possibly_invalid_spec() instead.

https://codereview.chromium.org/735863002/diff/20001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/735863002/diff/20001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:168: // If the URL was not valid, it's
not possible to get the spec.
I think you can use possibly_invalid_spec() here.

[azani, 2014-11-19 23:08:54]

https://codereview.chromium.org/735863002/diff/20001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/735863002/diff/20001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:168: // If the URL was not valid, it's
not possible to get the spec.
On 2014/11/19 20:29:28, Aaron Boodman wrote:
> I think you can use possibly_invalid_spec() here.

Nope. Also returns an empty string. It looks like if the URL cannot be parsed,
GURL simply leaves spec_ (the private variable it uses to hold the spec) as an
empty string. It looks like that is because the way spec_ is built is by parsing
the input_spec and then reconstructing a spec.

[Aaron Boodman, 2014-11-20 01:01:57]

In that case I guess just move the check to
ApplicationManager::ConnectToApplication() and convert the url param there to
type string.

[azani, 2014-11-20 19:15:58]

On 2014/11/20 01:01:57, Aaron Boodman wrote:
> In that case I guess just move the check to
> ApplicationManager::ConnectToApplication() and convert the url param there to
> type string.

Done. Some callers still take a GURL. Shall I refactor them to take a string
too? I'm not sure how far down this rabbit hole it makes sense to go.

[azani, 2014-11-20 21:24:41]

Moved the checks out as discussed. PTAL.

[Aaron Boodman, 2014-11-20 21:53:59]

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
mojo/application_manager/application_manager.cc:104: if (!app_gurl.is_valid() &&
app_gurl.possibly_invalid_spec().size() == 0) {
This is the first code that gets called when some other application calls
ApplicationManager::ConnectToApplication().

Since anyone could pass anything to that method, we don't want to consider this
a DCHECK-level error. We should validate it, print a nice error, but otherwise
continue gracefully.

When I brought up the DCHECK, I meant it should go in
ApplicationManager::ConnectToApplication(), not here. Because at that point,
higher layers should have already validated the GURL.

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
mojo/application_manager/application_manager.cc:105: LOG(ERROR) << "Error:
invalid URL: " << app_url;
return here. The InterfaceRequest will get destroyed and the caller will get an
error. This is the same thing that ApplicationManager is going to do later, but
if we get that far, AM is going to assert, which we don't want.

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
mojo/application_manager/application_manager.cc:107:
DCHECK(app_gurl.possibly_invalid_spec().size() > 0);
This moves to ApplicationManager::ConnectToApplication().

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/desktop/mojo_...
File mojo/shell/desktop/mojo_main.cc (right):

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/desktop/mojo_...
mojo/shell/desktop/mojo_main.cc:61: LOG(ERROR) << "Error: invalid URL: " <<
argv[0];
same feedback as in other file.

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:214:
DCHECK(content_handler_url.possibly_invalid_spec().size() > 0)
DCHECK(content_handler_url.is_valid())

[azani, 2014-11-20 22:10:03]

ptal

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
mojo/application_manager/application_manager.cc:104: if (!app_gurl.is_valid() &&
app_gurl.possibly_invalid_spec().size() == 0) {
On 2014/11/20 21:53:59, Aaron Boodman wrote:
> This is the first code that gets called when some other application calls
> ApplicationManager::ConnectToApplication().
> 
> Since anyone could pass anything to that method, we don't want to consider
this
> a DCHECK-level error. We should validate it, print a nice error, but otherwise
> continue gracefully.
> 
> When I brought up the DCHECK, I meant it should go in
> ApplicationManager::ConnectToApplication(), not here. Because at that point,
> higher layers should have already validated the GURL.

Done.

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
mojo/application_manager/application_manager.cc:105: LOG(ERROR) << "Error:
invalid URL: " << app_url;
On 2014/11/20 21:53:59, Aaron Boodman wrote:
> return here. The InterfaceRequest will get destroyed and the caller will get
an
> error. This is the same thing that ApplicationManager is going to do later,
but
> if we get that far, AM is going to assert, which we don't want.

Done.

https://codereview.chromium.org/735863002/diff/60001/mojo/application_manager...
mojo/application_manager/application_manager.cc:107:
DCHECK(app_gurl.possibly_invalid_spec().size() > 0);
On 2014/11/20 21:53:59, Aaron Boodman wrote:
> This moves to ApplicationManager::ConnectToApplication().

Done.

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/desktop/mojo_...
File mojo/shell/desktop/mojo_main.cc (right):

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/desktop/mojo_...
mojo/shell/desktop/mojo_main.cc:61: LOG(ERROR) << "Error: invalid URL: " <<
argv[0];
On 2014/11/20 21:53:59, Aaron Boodman wrote:
> same feedback as in other file.

Done.

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/735863002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:214:
DCHECK(content_handler_url.possibly_invalid_spec().size() > 0)
On 2014/11/20 21:53:59, Aaron Boodman wrote:
> DCHECK(content_handler_url.is_valid())

Done.

[Aaron Boodman, 2014-11-20 22:16:03]

https://codereview.chromium.org/735863002/diff/80001/mojo/shell/desktop/mojo_...
File mojo/shell/desktop/mojo_main.cc (right):

https://codereview.chromium.org/735863002/diff/80001/mojo/shell/desktop/mojo_...
mojo/shell/desktop/mojo_main.cc:61: LOG(ERROR) << "Error: invalid URL: " <<
argv[0];
Add a return here. We don't want to set any args for an invalid URL.

[azani, 2014-11-20 22:18:00]

ptal

https://codereview.chromium.org/735863002/diff/80001/mojo/shell/desktop/mojo_...
File mojo/shell/desktop/mojo_main.cc (right):

https://codereview.chromium.org/735863002/diff/80001/mojo/shell/desktop/mojo_...
mojo/shell/desktop/mojo_main.cc:61: LOG(ERROR) << "Error: invalid URL: " <<
argv[0];
On 2014/11/20 22:16:02, Aaron Boodman wrote:
> Add a return here. We don't want to set any args for an invalid URL.

Done.

[Aaron Boodman, 2014-11-20 22:19:16]

lgtm. make sure the tests pass locally before you land (we don't have try bots).

[azani, 2014-11-21 18:02:38]

Committed patchset #7 (id:120001) manually as
22190df420651906c772fe7bda993809d58bc1fd (presubmit successful).