This affects all chrome.fileSystemProvider applications. I'm pretty sure this landed before the branch point, so this regressing is in Chrome 69 - so we might want to consider merging the revert.

I think this might even repro for local files, but since I don't have any logging there I can't be sure. But when navigating from a dir with >20k files even the local file system hangs.

Essentially when navigating away from a directory, before it starts the readDir, foreach file getmetadata sequence for the new directory, it redundantly calls getmetadata on every file in the current directory (which was already fetched) before it starts on the new directory.

Since we are already in this directory, we already read all the metadata so there should be no need to read it all again.

Since all these redundant calls have to be serviced before the readDir for the new directory, the UI will spin. For the SMB provider this amounts to a 5-10 second hang per 1000 files in the *current* directory even if the new directory is empty.

The degree to which all the other fileSystemProvider implementations are affect depends on how expensive metadata requests are for them.

You can repro by putting logging in the implementations of ProvidedFileSystemInterface - I think there are currently 2, one for our Native SMB implementation, and one that proxies to javascript app implementations.

Also looks like there is a fake implementation of this interface that you could try to use

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h?sq=package:chromium&g=0

If you want to reproduce specifically with SMB. You'll need a NAS device that supports SMB2 or higher, or a windows machine with a share and firewall configured appropriately.

Enable chrome://flags#smb-native
Go to the files app, 
Click Add New Service->SMB Shares. 
Enter an smb url eg. \\server\share or smb://server/share
Mount the share

Test Case 1:
Navigate to a folder that contains at least 5000 files
For our SMB provider - wait >30 seconds to ensure we invalidated our cache. For other providers their caching behavior may vary.
Navigate to a folder with 1 file

The UI will spin for 30 seconds or so before displaying the file.

Test Case 2:
Navigate to an empty folder
Navigate to a folder with 1 file

This will respond in <10ms

By varying the size of the starting folder, you can affect how long the UI hangs.

Sam - FYI

lucmult@ - Do you have the bandwidth to take a look at this?

Yes, yesterday I started looking into this with noel@.

We have identified a couple of potential improvements we can do, but we're now measuring the actual impact of these potential improvements.

I'll focus on this measurement today.

From: https://chromium-review.googlesource.com/c/chromium/src/+/1158994

Relevant here is that this isn't doing what it says it is doing since newDirContents actually contains the previous directory.

There seems to be something more generally wrong with this function since as far as I can tell whenever this gets called during a directory change newDirContents is equal to this.currentDirContents_ (ie. it's the old content). so eg line 500 doesn't really do anything (I didn't veriify the entire data structure, but at least the fileList is the old content)

this.currentDirContents_ doesn't get reset until the call to this.scan_ on line 574, and then isn't populated with the new content until sometime between the call to scan_ and the callback to onUpdated on line 534.

So if there is logic that relies on what line 567 is doing, it is most likely basing that logic on information from the wrong directory.

So I'd suspect you can write a test that first goes to directory A that has one set of permissions, then navigate to directory B that has a different set and directory B and directory B will be using the permission set from A.

If this is intended to run on the new directory it really needs to be in onUpdated, though I suspect by that point it is redundant.

It's possible that there is actually a race condition (either in the tests or in the real code) where whatever is checking the permissions is expecting to be able to do it after this function ends, ie. on scan-started, when it needs to wait until scan-updated.

Here's a repro for the regression to existing fileSystemProviders. Most likely affected are zip, mtp, the existing SMB extension, drop box extension.

Download this zip file from drive.  
https://drive.google.com/open?id=0B_8E6lwmrIbVWl9SbjFHRl9tbXVheGh1MGhOU0dNRWl4d29r

Double click the zip file to "mount" it

Navigate to the directory "20000" - wait for it all to finish
Navigate to the directory "1".

Before this change: navigation to "1" is instant.

After this change: the UI completely freezes, animations stop or jank, and both the before and after selection highlight are visible at the same time for 2-3 seconds. Then the UI "spinner" bar shows and it takes about 7 seconds total to display the file.

Regarding "newDirContents". The name is actually misleading. Now that I traced the code a bit more it is better thought of as "containerToManipulateForNextDirectoryChange". 

It actually gets initialized using the current directories content, so my comment in #7 about line 500 being redundant is not exactly correct. It's a different instance but it is initialized with the same fileList as the current one.

The problem seems to be that the permission check is expecting the scan to be synchronous rather than waiting for the callback/event. The new directory contents are not available at this point in time.

What I've learnt so far is basically what zentaro@ described above, the "newDirContents" part is misleading and not doing what it was intending. The explanation for the tests that started failing once the code was removed is that the test actually works due to a side-effect of that "mistake" of the "newDirContents" part.

The test goes like:

1. Open My Drive.
2. Navigate to children folder "Read-Only Folder".
3. Check if the "Read-Only Folder" menus are correct.

At step #2, the misbehaving code will fetch the metadata for "My Drive"'s content, which actually includes "Read-Only Folder" thus the test "works".  If the test used a sub-folder without navigating to its parent it wouldn't work.

This odd behavior probably explains other bugs like this crbug.com/866650 where a division line sometimes appears on the menu but sometimes doesn't.

I have to investigate the code that relies on the metadata to disable/enable menus to see if we can make it async without increasing the number of requests to metadata providers, because if we add more calls to our current async API it will still send multiple requests to backend.

----
I've wrote a test to assess the impact of how many metadata requests that particular code issues:

For a test that navigaties to [Drive or Downloads], expand a child, expand a gran-child and then select gran-child we get the following numbers:

ToT: 
 - Downloads: fromCache: 0, fullFetch: 51
 - Drive:     fromCache: 8, fullFetch: 37

Without that code:
 - Downloads: fromCache: 0, fullFetch: 40   ~21% less requests
 - Drive:     fromCache: 8, fullFetch: 26   ~30% less requests

There is another part invalidating the cache for all properties where it only wants the fresh "share" property which can improve our cache utilization too. Our current implementation for Metadata and its cache doesn't provide a simple way to refresh only 1 property, so we invalidate all properties and fetch them back.

------

I'm focusing now on try to make the menus work in async way without a huge refactor to be able to backport to M-69.

Worst case you should be able to filter on RootType. Since this is detrimental to all file systems and you only care about its accidental side effect for drive you could at least skip this call for everything else.

We get the similar fix to crrev.com/c/1158994, however we fixed to make the tests pass.

The fix is still simple enough for easy merge on M-69.

We're still investigating other parts to either reduce the number requests or improve the use of metadata.

https://chromium-review.googlesource.com/c/chromium/src/+/1161843

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76d39f0f17047ddacdcbf4c249e45eb047fd96ee

commit 76d39f0f17047ddacdcbf4c249e45eb047fd96ee
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Fri Aug 03 12:16:05 2018

Stop sending unnecessary metadata requests to FS providers

Update DirectoryModel.clearAndScan_ to stop requesting previous
directory's content metadata, that caused  issue 867974  where we were
sending too many metadata requests.

However we still need to fetch metadata for the currently selected
directory so is available to use by menu/commands.

Change the File Manager CommandHandler to update all commands
when the metadata gets updated. This fixes the commands that should be
disabled based on metadata properties canCopy, canCut, can*.

Bug:  867974 
Change-Id: I5254f70b8993a6e9b536f53543d1f3ed2ca0d87f
Reviewed-on: https://chromium-review.googlesource.com/1161843
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Stuart Langley <slangley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580511}
[modify] https://crrev.com/76d39f0f17047ddacdcbf4c249e45eb047fd96ee/ui/file_manager/externs/command_handler_deps.js
[modify] https://crrev.com/76d39f0f17047ddacdcbf4c249e45eb047fd96ee/ui/file_manager/file_manager/foreground/js/directory_model.js
[modify] https://crrev.com/76d39f0f17047ddacdcbf4c249e45eb047fd96ee/ui/file_manager/file_manager/foreground/js/file_manager_commands.js

Requesting to merge on M-69 branch.

This bug is a performance regression that affects M-69 branch.

I've tested manually by flashing a new test image in a device and compiled M-69 branch (branch-heads/3497) with 2 additional patches:
- crrev.com/c/1161843  - Being requested to merge here
- crrev.com/c/1139944  - Requesting merge in  crbug.com/682356 

Attached is a screencast of my navigation test.

Deleted: navigate-between-folders-add-new-service 1.6 MB

navigate-between-folders-add-new-service 1.6 MB View Download

This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The patch requested to merge also fix another bug crbug.com/866650.

Zentaro/Bailey,

Can you verify this patch fixes the issue you've observed?

Thanks

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c9f8e91ed36f002427d0cb280818c59aad278834

commit c9f8e91ed36f002427d0cb280818c59aad278834
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Tue Aug 07 08:48:54 2018

Don't fetch metadata for FakeEntry

Metadata backend raise an error if we pass a non-Entry object. This
fixes an error when clicking on "Drive > Shared with me".

Bug:  867974 
Change-Id: I741c310c86b1481823dc9fa5bd84575050c428d3
Reviewed-on: https://chromium-review.googlesource.com/1164802
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581170}
[modify] https://crrev.com/c9f8e91ed36f002427d0cb280818c59aad278834/ui/file_manager/file_manager/foreground/js/directory_model.js

slangley@ - Based on the fact that the fix removes the line of code we identified, we're pretty confident it does. But I will test with latest ToT today to confirm.

Re-iterating the merge request on M-69:

Patches:
- crrev.com/c/1161843 
- crrev.com/c/1164802

They're a fix for a performance regression.

Confirmed testing via IM, lucmult. Approving merge for M69.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5323bcf2e09ea88192f5db012b90605ba6cf537

commit d5323bcf2e09ea88192f5db012b90605ba6cf537
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Wed Aug 08 02:08:36 2018

Stop sending unnecessary metadata requests to FS providers

Update DirectoryModel.clearAndScan_ to stop requesting previous
directory's content metadata, that caused  issue 867974  where we were
sending too many metadata requests.

However we still need to fetch metadata for the currently selected
directory so is available to use by menu/commands.

Change the File Manager CommandHandler to update all commands
when the metadata gets updated. This fixes the commands that should be
disabled based on metadata properties canCopy, canCut, can*.

Bug:  867974 
Change-Id: I5254f70b8993a6e9b536f53543d1f3ed2ca0d87f
Reviewed-on: https://chromium-review.googlesource.com/1161843
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Stuart Langley <slangley@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#580511}(cherry picked from commit 76d39f0f17047ddacdcbf4c249e45eb047fd96ee)
Reviewed-on: https://chromium-review.googlesource.com/1166483
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#483}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/d5323bcf2e09ea88192f5db012b90605ba6cf537/ui/file_manager/externs/command_handler_deps.js
[modify] https://crrev.com/d5323bcf2e09ea88192f5db012b90605ba6cf537/ui/file_manager/file_manager/foreground/js/directory_model.js
[modify] https://crrev.com/d5323bcf2e09ea88192f5db012b90605ba6cf537/ui/file_manager/file_manager/foreground/js/file_manager_commands.js

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c0f57b3cd45027d6e16b1286c280db67aebaf3b

commit 8c0f57b3cd45027d6e16b1286c280db67aebaf3b
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Wed Aug 08 02:10:06 2018

Don't fetch metadata for FakeEntry

Metadata backend raise an error if we pass a non-Entry object. This
fixes an error when clicking on "Drive > Shared with me".

Bug:  867974 
Change-Id: I741c310c86b1481823dc9fa5bd84575050c428d3
Reviewed-on: https://chromium-review.googlesource.com/1164802
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581170}(cherry picked from commit c9f8e91ed36f002427d0cb280818c59aad278834)
Reviewed-on: https://chromium-review.googlesource.com/1166363
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#484}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/8c0f57b3cd45027d6e16b1286c280db67aebaf3b/ui/file_manager/file_manager/foreground/js/directory_model.js

I guess Chrome hasn't been uprev'd lately, but building ToT Chrome OS without also deploying Chrome still seems to be getting the old code.

I verified that this is fixed.

Thanks Zentaro.