Fixed "blocking io" from FixupPath on UI thread.

Fixed "blocking io" from FixupURL on UI thread.

FixupURL calls FixupPath who then calls FilePathToFileURL, in which adding
current directory in specific cases (details below) was removed.
That caused DCHECK because getting cwd is protected with AssertIOAlolowed.
Code being removed was reachable from autocomlete, which resulted in
DCHECK. So it was responsible for some "functionallity", but not useful.
I examined, which functionality we will lose: specific paths like "c:",
"с::foo", "c:foo" are recognized as not absolute paths on Windows and cwd
is added. What happens for those URLs: AutocompleteInput::Parse calls
FixupURL, in there SegmentURLInternal recognizes them as having file
scheme on Windows at the same time segmenting to parts is not successful
(url::Parsed is not valid). This functionality is only reachable on
Windows and it is not useful, because those paths with ":" are not valid
relative paths on Windows. For example, autocomlete result like
file:///C:/Users/me/chromium/src/out/Debug/C: (cwd added before C:)
isn't useful.

For other paths of that style cwd-adding code was not called:
  "file://c:", "file://./foo" are segmented successfully
    by SegmentURLInternal, so, no current dirrectory adding here.
  "c:/foo", "c:\foo" - are recognoized as absolute paths, no CWD added
  "./foo.txt", "foo.txt" "foo\foo.txt" - not recognized as having "file"
    scheme, no CWD added.
  ~/directory_or_file URLs - don't need CWD.

Regarding existing functionality working with relative paths:
GetURLsFromCommandLine and ProfileImpl::GetHomePage that can get
relative path(or homepage URL) from command line both use
not FixupURL, FixupRelativeFile function.
FilePathToFileURL is documented in it's comment as taking absolute path
as argument. Also FixupURL (which indirectly calls FilePathToFileURL),
by it's name and description, is not intended to fixup relative file paths.
Not to mention, resolving relative paths to absolute via FixupURL is broken.

Regarding client code that could use functionality being removed
in this fix. Adding cwd was introduced in commit
0135aa172211433181e1cdaf009ff77716e87ff7
https://codereview.chromium.org/137233006
that time FilePathToFileURL was in net_util.cc.
Client code mentioned there is content_shell
Code working with startup URL from there:
 src\content\shell\browser\shell_browser_main_parts.cc(75):
   return net::FilePathToFileURL(base::FilePath(args[0]));
Now this code is not included in project (don't know why).
This particular client code needs CWD adding, it was fixed to use right
function for adding cwd.

R=davidben@chromium.org,pkasting@chromium.org,alekseys@chromium.org,gunsch@chromium.org
BUG=60641
TEST=entering "c:" url will not cause debug version to fail on DCHECK
on Windows, and launching content_shell with argument like foo.html
(this html shouldexist in current diroctory) results in foo.html
contents shown in content_shell and absolute path to foo.html
displayed in addres bar.

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

[davidben, 2015-04-07 18:11:36]

not lgtm. I don't think this change is correct. Even with a lock,
GetCurrentDirectoryThreadSafe isn't thread-safe. If I GetCurrentDirectory (or
even try to resolve a relative path!) while racing with a thread that's trying
to SetCurrentDirectory, I have a race anyway, regardless of whether setting the
actual global variable causes issues. Consider racing with
url_fixer::FixupRelativeFile between when the cwd is changed and restored.

https://code.google.com/p/chromium/codesearch#chromium/src/components/url_fix...

I'm not quite sure what would be the right way to set up asserts here (I agree
AssertIOAllowed is dubious). We should either ensure that only one thread is
allowed to deal with relative paths and cwd or ensure that SetCurrentDirectory
is never called after a process spawns multiple threads (though after fork() is
fine on UNIX). The latter seems more tenable since there aren't that many calls.
Maybe it should be renamed to SetCurrentDirectoryUnsafe and existing uses
post-thread-spawn fixed.

https://code.google.com/p/chromium/codesearch#search/&q=SetCurrentDirectory&s...


At a glance, it looks like base/native_library_win.cc and
components/url_fixer/url_fixer.cc are potentially problematic. A third
possibility is that you actually need to expose the mutex and have those two
files wrap their entire bodies around the mutex. But taking that lock then badly
needs AssertIOAllowed since we'll block, so url_fixer.cc should arrange to not
need it (which seems to be mentioned in the bug).

[eugenebng, 2015-04-13 14:48:49]

eugenebng@yandex-team.ru changed reviewers:
+ pkasting@chromium.org
- rvargas@chromium.org

[eugenebng, 2015-04-14 11:06:51]

Thanks for your review. I've uploaded another patch set and changed description.

[davidben, 2015-04-14 16:26:22]

> So, while AssertIOAlolowed in Get\SetCurrentDirectory is doubtful, it prevents
race by allowing Get\Set current dir only in IO thread

That's not what AssertIOAllowed does. The IO thread is *not* allowed to do file
IO. And there are other threads in Chromium which *are* allowed to do file IO.


I don't think splitting that function is the right way to go here. We should
figure out where relative paths are resolved and determine (a) what
functionality is lost and (b) is there a better place to move the relative path
lookups.

See discussion on the bug. I like the suggestion to, e.g., make paths absolute
early when passed as command-line arguments.
https://code.google.com/p/chromium/issues/detail?id=60641#c18

https://codereview.chromium.org/1068793002/diff/20001/components/url_fixer/ur...
File components/url_fixer/url_fixer.cc (right):

https://codereview.chromium.org/1068793002/diff/20001/components/url_fixer/ur...
components/url_fixer/url_fixer.cc:181: GURL file_url =
net::FilePathToFileURLNoCWD(base::FilePath(filename));
This is a loss of functionality for this function. Did you verify that no caller
of this function ever needs relative paths?

https://codereview.chromium.org/1068793002/diff/20001/net/base/filename_util.cc
File net/base/filename_util.cc (right):

https://codereview.chromium.org/1068793002/diff/20001/net/base/filename_util....
net/base/filename_util.cc:94: #endif
This is too much code to be duplicated. Move things to a helper function.

https://codereview.chromium.org/1068793002/diff/20001/net/base/filename_util.h
File net/base/filename_util.h (right):

https://codereview.chromium.org/1068793002/diff/20001/net/base/filename_util....
net/base/filename_util.h:24: // will indirectly do AssertIOAllowed
I think there may be some confusion here. AssertIOAllowed does not mean this is
only callable on the IO thread. The IO thread is the IPC and network IO thread
and should not have file IO.

[Peter Kasting, 2015-04-14 20:46:09]

On 2015/04/14 16:26:22, David Benjamin wrote:
> I don't think splitting that function is the right way to go here. We should
> figure out where relative paths are resolved and determine (a) what
> functionality is lost and (b) is there a better place to move the relative
path
> lookups.
> 
> See discussion on the bug. I like the suggestion to, e.g., make paths absolute
> early when passed as command-line arguments.
> https://code.google.com/p/chromium/issues/detail?id=60641#c18

Yes, comments 19 and 20 on that bug are almost certainly the correct fix here.

[eugenebng, 2015-04-20 14:28:38]

On 2015/04/14 20:46:09, Peter Kasting wrote:
> On 2015/04/14 16:26:22, David Benjamin wrote:
> > I don't think splitting that function is the right way to go here. We should
> > figure out where relative paths are resolved and determine (a) what
> > functionality is lost and (b) is there a better place to move the relative
> path
> > lookups.
> > 
> > See discussion on the bug. I like the suggestion to, e.g., make paths
absolute
> > early when passed as command-line arguments.
> > https://code.google.com/p/chromium/issues/detail?id=60641#c18
> 
> Yes, comments 19 and 20 on that bug are almost certainly the correct fix here.

What i figured out based on that - please, see updated description & new patch
set.

[eugenebng, 2015-04-20 15:02:20]

On 2015/04/20 14:28:38, eugenebng wrote:
> On 2015/04/14 20:46:09, Peter Kasting wrote:
> > On 2015/04/14 16:26:22, David Benjamin wrote:
> > > I don't think splitting that function is the right way to go here. We
should
> > > figure out where relative paths are resolved and determine (a) what
> > > functionality is lost and (b) is there a better place to move the relative
> > path
> > > lookups.
> > > 
> > > See discussion on the bug. I like the suggestion to, e.g., make paths
> absolute
> > > early when passed as command-line arguments.
> > > https://code.google.com/p/chromium/issues/detail?id=60641#c18
> > 
> > Yes, comments 19 and 20 on that bug are almost certainly the correct fix
here.
> 
> What i figured out based on that - please, see updated description & new patch
> set.

Regarding autocompleting paths in addres bar/omnibox/etc in other browsers: I
checked IE and Firefox and haven't found working relative path autocomplete. But
IE does autocomplete absolute file paths, eg, offers "c:\Program files" and
"C:\Program files(x86)" for "c:\Progr" input. However, there is no such
functionality on Chromium now (if not counting broken functionality mentioned in
Description). And if such functionality will ever be added (with relative paths
handling), it should use FixupRelativeFile rether then FixupURL or
FilePathTofileURL - please, see Description for details.

[Peter Kasting, 2015-04-20 22:06:56]

I'm not totally clear from your description, but it seems like removing this
re-breaks bug 336416, and to avoid that, we need to change GetStartupURL() to
explicitly resolve relative paths.

[eugenebng, 2015-04-21 21:31:47]

On 2015/04/20 22:06:56, Peter Kasting wrote:
> I'm not totally clear from your description, but it seems like removing this
> re-breaks bug 336416, and to avoid that, we need to change GetStartupURL() to
> explicitly resolve relative paths.

Thats'right, content_shell was damaged by removing adding CWD from
FilePathToFileURL. Fixed GetStartupURL(), so cwd is added right there.

[Peter Kasting, 2015-04-21 22:34:05]

LGTM

https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
content/shell/browser/shell_browser_main_parts.cc:69:
base::MakeAbsoluteFilePath(base::FilePath(args[0])));
Nit: This would probably be safer as:

  base::FilePath file_path(base::MakeAbsoluteFilePath(base::FilePath(args[0])));
  return file_path.empty() ? GURL() : net::FilePathToFileURL(file_path);

[davidben, 2015-04-22 01:09:18]

I looked through git log -SFilePathToFileURL from when this behavior was added
to now. Most of it looks like it doesn't care, but I find two places which
might.

https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
content/shell/browser/shell_browser_main_parts.cc:68: return
net::FilePathToFileURL(
This change ought to be mirrored in
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...

Probably also here, though it seems to be a random manual test.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...

[eugenebng, 2015-04-22 21:01:52]

eugenebng@yandex-team.ru changed reviewers:
+ alekseys@chromium.org, gunsch@chromium.org

[eugenebng, 2015-04-22 21:02:25]

On 2015/04/22 01:09:18, David Benjamin (OOO sick) wrote:
> I looked through git log -SFilePathToFileURL from when this behavior was added
> to now. Most of it looks like it doesn't care, but I find two places which
> might.
> 
>
https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
> File content/shell/browser/shell_browser_main_parts.cc (right):
> 
>
https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
> content/shell/browser/shell_browser_main_parts.cc:68: return
> net::FilePathToFileURL(
> This change ought to be mirrored in
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...
> 
> Probably also here, though it seems to be a random manual test.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...

Done

[Aleksey Shlyapnikov, 2015-04-22 22:34:19]

lgtm

[Peter Kasting, 2015-04-22 22:40:04]

Thanks for fixing these other locations.

https://codereview.chromium.org/1068793002/diff/80001/chromecast/browser/serv...
File chromecast/browser/service/cast_service_simple.cc (right):

https://codereview.chromium.org/1068793002/diff/80001/chromecast/browser/serv...
chromecast/browser/service/cast_service_simple.cc:30: return
net::FilePathToFileURL(
Both this and shell_browser_main_parts.cc should be using the safer form I noted
in my last review comments, which correctly handles the case where an invalid
path is passed in.

https://codereview.chromium.org/1068793002/diff/80001/chromecast/browser/serv...
chromecast/browser/service/cast_service_simple.cc:72:
web_contents_->GetController().LoadURL(startup_url_, content::Referrer(),
This call should be guarded by an "if (startup_url_.is_valid()) {" check.

https://codereview.chromium.org/1068793002/diff/80001/content/shell/browser/s...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1068793002/diff/80001/content/shell/browser/s...
content/shell/browser/shell_browser_main_parts.cc:127: GetStartupURL(),
For maximum safety, can you also modify Shell::CreateNewWindow() so that instead
of checking "if (!url.is_empty())" it checks "if (url.is_valid())"?

[eugenebng, 2015-04-23 12:01:52]

Regarding code:
> base::FilePath file_path(base::MakeAbsoluteFilePath(base::FilePath(args[0])));
> return file_path.empty() ? GURL() : net::FilePathToFileURL(file_path);
I used FilePathToFileURL(base::MakeAbsoluteFilePath(...)) because this solution
is already used in content_shell. 
If code returning empty URL for empty path is better, wouldn't it be good to
change that everywhere in conntent_shell as another issue?

https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1068793002/diff/60001/content/shell/browser/s...
content/shell/browser/shell_browser_main_parts.cc:69:
base::MakeAbsoluteFilePath(base::FilePath(args[0])));
On 2015/04/21 22:34:05, Peter Kasting wrote:
> Nit: This would probably be safer as:
> 
>   base::FilePath
file_path(base::MakeAbsoluteFilePath(base::FilePath(args[0])));
>   return file_path.empty() ? GURL() : net::FilePathToFileURL(file_path);

I used FilePathToFileURL(base::MakeAbsoluteFilePath(...)) because this solution
is already used in content_shell. 
If code returning empty URL for empty path is better, wouldn't it be good to
change that everywhere in conntent_shell as another issue?

[davidben, 2015-04-23 17:51:07]

lgtm

[Peter Kasting, 2015-04-23 22:46:07]

On 2015/04/23 12:01:52, eugenebng wrote:
> Regarding code:
> > base::FilePath
file_path(base::MakeAbsoluteFilePath(base::FilePath(args[0])));
> > return file_path.empty() ? GURL() : net::FilePathToFileURL(file_path);
> I used FilePathToFileURL(base::MakeAbsoluteFilePath(...)) because this
solution
> is already used in content_shell. 
> If code returning empty URL for empty path is better, wouldn't it be good to
> change that everywhere in conntent_shell as another issue?

If there's lots of code doing this, then the alternative would be for
FilePathToFileURL() to detect empty/invalid FilePaths on input and return GURL()
in response.

Unless, that is, GURL's constructor already results in an empty GURL when called
with an invalid input string, in which case no changes are necessary.  You
should debug this to find out what happens in such a case.

[gunsch, 2015-04-28 04:32:06]

sorry, just noticed this: chromecast/ lgtm

[eugenebng, 2015-04-28 10:20:42]

The CQ bit was checked by eugenebng@yandex-team.ru

[eugenebng, 2015-04-28 10:20:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1068793002/#ps80001
(title: "Fixed adding cwd aka "blocking IO" used in FixupURL via
FilePathToFileURL, also fixed client code")

[commit-bot: I haz the power, 2015-04-28 10:20:46]

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

[commit-bot: I haz the power, 2015-04-28 11:18:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-28 11:19:12]

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

[eugenebng, 2015-04-28 13:23:02]

On 2015/04/23 22:46:07, Peter Kasting wrote:
> On 2015/04/23 12:01:52, eugenebng wrote:
> > Regarding code:
> > > base::FilePath
> file_path(base::MakeAbsoluteFilePath(base::FilePath(args[0])));
> > > return file_path.empty() ? GURL() : net::FilePathToFileURL(file_path);
> > I used FilePathToFileURL(base::MakeAbsoluteFilePath(...)) because this
> solution
> > is already used in content_shell. 
> > If code returning empty URL for empty path is better, wouldn't it be good to
> > change that everywhere in conntent_shell as another issue?
> 
> If there's lots of code doing this, then the alternative would be for
> FilePathToFileURL() to detect empty/invalid FilePaths on input and return
GURL()
> in response.
> 
> Unless, that is, GURL's constructor already results in an empty GURL when
called
> with an invalid input string, in which case no changes are necessary.  You
> should debug this to find out what happens in such a case.

FilePathToFileURL returns non-empty URL (file:///) when called with empty path
argument. This URL is valid, and on posix it even exists (local root dir).
Returning it for empy argument doesn't seem to be correct. At a glance, client
code mostly doesn't check if path is empty. So it looks like a dood idea to
detect empty paths in FilePathToFileURL.
About invalid paths - FilePathToFileURL resolves FilePath(L"etc") argument to
file:///etc, which is again existing local directory URL. So checking arcgument
to be valid full paths in FilePathToFileURL looks right too. 
Perhaps, it's better to create start another bug/issue about that?

[Peter Kasting, 2015-04-28 20:47:15]

On 2015/04/28 13:23:02, eugenebng wrote:
> FilePathToFileURL returns non-empty URL (file:///) when called with empty path
> argument. This URL is valid, and on posix it even exists (local root dir).
> Returning it for empy argument doesn't seem to be correct. At a glance, client
> code mostly doesn't check if path is empty. So it looks like a dood idea to
> detect empty paths in FilePathToFileURL.

That's a reasonable variant of what I proposed, and perhaps safer and easier to
implement than changing callers.

> About invalid paths - FilePathToFileURL resolves FilePath(L"etc") argument to
> file:///etc, which is again existing local directory URL. So checking
arcgument
> to be valid full paths in FilePathToFileURL looks right too.

Do you mean just checking whether the input string looks like a full path, or
actually checking whether it exists?  The former is probably OK, the latter we
want to avoid (requires disk access).

I would open a new bug for these.

[eugenebng, 2015-04-29 15:11:49]

On 2015/04/28 20:47:15, Peter Kasting wrote:
> On 2015/04/28 13:23:02, eugenebng wrote:
> > FilePathToFileURL returns non-empty URL (file:///) when called with empty
path
> > argument. This URL is valid, and on posix it even exists (local root dir).
> > Returning it for empy argument doesn't seem to be correct. At a glance,
client
> > code mostly doesn't check if path is empty. So it looks like a dood idea to
> > detect empty paths in FilePathToFileURL.
> 
> That's a reasonable variant of what I proposed, and perhaps safer and easier
to
> implement than changing callers.
> 
> > About invalid paths - FilePathToFileURL resolves FilePath(L"etc") argument
to
> > file:///etc, which is again existing local directory URL. So checking
> arcgument
> > to be valid full paths in FilePathToFileURL looks right too.
> 
> Do you mean just checking whether the input string looks like a full path, or
> actually checking whether it exists?  The former is probably OK, the latter we
> want to avoid (requires disk access).
Yes, I mean the former, not the latter, mentioned that in the new issue too.  
> 
> I would open a new bug for these.

Created issue: https://code.google.com/p/chromium/issues/detail?id=482481

[eugenebng, 2015-05-13 12:36:21]

found a copy of the code changed in this issue here:
/src/mojo/util/filename_util.cc and h. Deosn't it look like I shod submit the
same change (remove adding CWD in FilePathTofileURL) there too?

[Peter Kasting, 2015-05-13 22:58:15]

On 2015/05/13 12:36:21, eugenebng wrote:
> found a copy of the code changed in this issue here:
> /src/mojo/util/filename_util.cc and h. Deosn't it look like I shod submit the
> same change (remove adding CWD in FilePathTofileURL) there too?

I would assume so?