Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

BUG=626972
Committed: https://crrev.com/5d555d2d08cab96acb183bd6032a8adcc4ceb3e5
Cr-Commit-Position: refs/heads/master@{#408465}

[Łukasz Anforowicz, 2016-07-26 22:30:50]

+jam@ to CC (as this is related to a CL under his review -
https://crrev.com/2175933002 - More aggressive IO asserts in
content_browsertests).

https://codereview.chromium.org/2168403003/diff/1/chrome/browser/download/sav...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/2168403003/diff/1/chrome/browser/download/sav...
chrome/browser/download/save_page_browsertest.cc:733:
base::ThreadRestrictions::SetIOAllowed(old_io_allowed_value);
I've reverted these changes in a more recent patchset - I am instead doing the
IO asserts on a broader scale in https://codereview.chromium.org/2175933002/
(this is for content_browsertests;  if this goes well, then maybe I'll also try
chrome layer's browser_tests in https://codereview.chromium.org/2175933002).

[Łukasz Anforowicz, 2016-07-27 00:04:05]

lukasza@chromium.org changed reviewers:
+ nasko@chromium.org

[Łukasz Anforowicz, 2016-07-27 00:04:05]

Nasko, can you take a look please?

Normally, I would ask Randy or Asanka for a review of
//content/browser/downloads stuff, but they are currently OOO, and I'd like to
move forward with the content_browsertests-wide IO asserts already stamped by
jam@ in https://codereview.chromium.org/2175933002/.

This CL doesn't really change much - SavePackage::GetSaveInfo method that
executes on the UI thread was already bouncing some stuff off of the FILE thread
via BrowserThread::PostTaskAndReplyWithResult(...,
&SavePackage::CreateDirectoryOnFileThread ...
&SavePackage::ContinueGetSaveInfo).  In the CL, I am just moving a few more
statements from GetSaveInfo method into CreateDirectoryOnFileThread method.  The
moved statements require file IO to convert mime type into suggested extension
(full callstack and more details are in the linked bug).

[jam, 2016-07-27 00:16:26]

Description was changed from

==========
Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

BUG=626972
==========

to

==========
Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

BUG=626972
==========

[jam, 2016-07-27 00:16:26]

jam@chromium.org changed reviewers:
+ thestig@chromium.org

[jam, 2016-07-27 00:18:13]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-07-27 00:18:14]

+thestig for a question about a really old cl:
https://codereview.chromium.org/6312195

That change added a threadrestriction check to these methods (i.e.
base::nix::GetFileMimeType). However the header wasn't updated and it still says
"Please note because it doesn't touch the disk, this does not
// work for directories.".

Lei: how come the above cl added the check? Is the comment in the header wrong
and this does make disk access? There's no bug linked in the cl for more
information.

[Lei Zhang, 2016-07-27 02:00:30]

On 2016/07/27 00:18:14, jam wrote:
> +thestig for a question about a really old cl:
> https://codereview.chromium.org/6312195
> 
> That change added a threadrestriction check to these methods (i.e.
> base::nix::GetFileMimeType). However the header wasn't updated and it still
says
> "Please note because it doesn't touch the disk, this does not
> // work for directories.".
> 
> Lei: how come the above cl added the check? Is the comment in the header wrong
> and this does make disk access? There's no bug linked in the cl for more
> information.

It's been a while, but my understanding is the comment for GetFileMimeType()
just says it does not actually read the given |filepath| param. Though the XDG
mime code needs to (re)read system mime configs from disk in order to work. Thus
calls to any XDG mime function can be slow and cause jank, even though it does
not touch the disk to read |filepath|.

[jam, 2016-07-27 16:33:45]

On 2016/07/27 02:00:30, Lei Zhang (Very Slow) wrote:
> On 2016/07/27 00:18:14, jam wrote:
> > +thestig for a question about a really old cl:
> > https://codereview.chromium.org/6312195
> > 
> > That change added a threadrestriction check to these methods (i.e.
> > base::nix::GetFileMimeType). However the header wasn't updated and it still
> says
> > "Please note because it doesn't touch the disk, this does not
> > // work for directories.".
> > 
> > Lei: how come the above cl added the check? Is the comment in the header
wrong
> > and this does make disk access? There's no bug linked in the cl for more
> > information.
> 
> It's been a while, but my understanding is the comment for GetFileMimeType()
> just says it does not actually read the given |filepath| param. Though the XDG
> mime code needs to (re)read system mime configs from disk in order to work.
Thus
> calls to any XDG mime function can be slow and cause jank, even though it does
> not touch the disk to read |filepath|.

ah, thanks for the explanation. In that case we should probably clarify the
comment.

lgtm (can you please also clarify the comment for GetFileMimeType in this
change?)

[nasko, 2016-07-27 17:00:20]

jam@ has already stamped it, but FWIW LGTM too.

[Łukasz Anforowicz, 2016-07-27 17:06:30]

thestig@, can you please take a quick look at the new comment in
base/nix/mime_util_xdg.h? (as an owner of //base + as the author of
https://codereview.chromium.org/6312195 :-).

On 2016/07/27 02:00:30, Lei Zhang (Very Slow) wrote:
> On 2016/07/27 00:18:14, jam wrote:
> > +thestig for a question about a really old cl:
> > https://codereview.chromium.org/6312195
> > 
> > That change added a threadrestriction check to these methods (i.e.
> > base::nix::GetFileMimeType). However the header wasn't updated and it still
> says
> > "Please note because it doesn't touch the disk, this does not
> > // work for directories.".
> > 
> > Lei: how come the above cl added the check? Is the comment in the header
wrong
> > and this does make disk access? There's no bug linked in the cl for more
> > information.
> 
> It's been a while, but my understanding is the comment for GetFileMimeType()
> just says it does not actually read the given |filepath| param. Though the XDG
> mime code needs to (re)read system mime configs from disk in order to work.
Thus
> calls to any XDG mime function can be slow and cause jank, even though it does
> not touch the disk to read |filepath|.

Thanks for raising this point.  In the latest patchset I've updated the doc
comment to explicitly say that:

1. |filepath| is not opened and doesn't have to exist (it doesn't exist when
base::nix::GetFileMimeType is called from
net::PlatformMimeUtil::GetPlatformMimeTypeFromExtension which passes a
non-existent |dummy_path|).

2. base::nix::GetFileMimeType can access the disk to read mime-type information
provided by the OS.  For example, the following callstack is possible:

fopen / fgets
_xdg_mime_glob_read_from_file
xdg_mime_init_from_directory
xdg_run_command_on_dirs 
xdg_mime_init
xdg_mime_get_mime_type_from_file_name [base/third_party/xdg_mime/xdgmime.c]
base::nix::GetFileMimeType [base/nix/mime_util_xdg.cc]

or

stat
xdg_mime_init_from_directory
...



FWIW, I've tried to audit other callers of base::nix::GetFileMimeType and they
all seem to work from the FILE thread:

- IconLoader::ReadGroupIDFromFilepath
- SelectFileDialogImplKDE::GetMimeTypeFilterString
- PlatformMimeUtil::GetPlatformMimeTypeFromExtension (although this one has a
lot of callers and I am not sure if I correctly understood all of them)

[Lei Zhang, 2016-07-28 18:31:29]

lgtm

[Łukasz Anforowicz, 2016-07-28 18:49:10]

Thanks for the reviews :-)

[Łukasz Anforowicz, 2016-07-28 18:49:14]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-07-28 18:49:15]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2168403003/#ps60001
(title: "Comment tweaks for base::nix::GetFileMimeType + two extra
DCHECK_CURRENTLY_ON asserts in SavePackag…")

[commit-bot: I haz the power, 2016-07-28 18:49:40]

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-07-28 20:35:59]

Description was changed from

==========
Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

BUG=626972
==========

to

==========
Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

BUG=626972
==========

[commit-bot: I haz the power, 2016-07-28 20:36:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-28 20:39:02]

Description was changed from

==========
Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

BUG=626972
==========

to

==========
Avoid file IO on UI thread by moving more save-page-as stuff to the FILE thread.

BUG=626972

Committed: https://crrev.com/5d555d2d08cab96acb183bd6032a8adcc4ceb3e5
Cr-Commit-Position: refs/heads/master@{#408465}
==========

[commit-bot: I haz the power, 2016-07-28 20:39:03]

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