assigning to test author: noel@chromium.org

Disabling test in release

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

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

commit 921ce9aaf50b49ce4f8852e86026d03fc71fc0e0
Author: Roger McFarlane <rogerm@chromium.org>
Date: Wed Jun 13 17:13:48 2018

[sheriff] Disable CopyBetweenWindows browser test in RELEASE

"CopyBetweenWindows/FilesAppBrowserTest.Test/copyBetweenWindowsLocalToUsb" is flaky.

We have detected 4 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyUwsSBUZsYWtlIkhDb3B5QmV0d2VlbldpbmRvd3MvRmlsZXNBcHBCcm93c2VyVGVzdC5UZXN0L2NvcHlCZXR3ZWVuV2luZG93c0xvY2FsVG9Vc2IM

TBR: noel@chromium.org
Bug:  852246 
Change-Id: I372fd33940e5780d74c6eb044b88c5a7b93f3802
Reviewed-on: https://chromium-review.googlesource.com/1099261
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Commit-Queue: Roger McFarlane <rogerm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566885}
[modify] https://crrev.com/921ce9aaf50b49ce4f8852e86026d03fc71fc0e0/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

This test has ben stable and non-flaky in RELEASE, for yonks.

Now marked flaky in RELEASE.  I suspect a recent Files.App change has introduced this flake (adding relevant peeps).

This link:

https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyUwsSBUZsYWtlIkhDb3B5QmV0d2VlbldpbmRvd3MvRmlsZXNBcHBCcm93c2VyVGVzdC5UZXN0L2NvcHlCZXR3ZWVuV2luZG93c0xvY2FsVG9Vc2IM 

provides the first clue:

 Try run at 2018-06-12 01:01:15 UTC 
   - looking for a patch just before crrev.com/566321

Symptoms:

*) CopyBetweenWindows/FilesAppBrowserTest.Test/copyBetweenWindowsLocalToUsb CRASH

:CONSOLE(5578)] "File system obtained: removable:fake-usb"
[:INFO:CONSOLE(296)] "Uncaught (in promise) TypeError: Cannot read property 'sort' of undefined", source: chrome-extension://oobinhbdbiehknkpbpejbbpdbkdjmoco/remote_call.js (296)

  - suspect array.sort() called on undefined.

*) GearMenu/FilesAppBrowserTest.Test/toogleGoogleDocsDrive CRASH

:CONSOLE(4402)] "Received the result of openMainWindow"
  - suspect sort() being called non-existent array, log also shows ...
[26776:26776:0612/010400.571577:INFO:CONSOLE(0)] "[FAIL] [toogleGoogleDocsDrive]: TypeError: Cannot read property 'sort' of undefined
    at chrome-extension://oobinhbdbiehknkpbpejbbpdbkdjmoco/remote_call.js:332:13

  - suspect array.sort() called on undefined.

*) CopyBetweenWindows/FilesAppBrowserTest.Test/copyBetweenWindowsLocalToUsb CRASH
:CONSOLE(5578)] "File system obtained: removable:fake-usb"
:INFO:CONSOLE(174)] "Uncaught (in promise) TypeError: Cannot read property 'length' of undefined", source: chrome-extension://oobinhbdbiehknkpbpejbbpdbkdjmoco/remote_call.js (174)

  - suspect .length called on undefined


*) Transfer/FilesAppBrowserTest.Test/transferFromOfflineToDownloads CRASH

"Received the result of selectVolume", source: chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj/background/js/background_common_scripts.js (4402)
[:INFO:CONSOLE(296)] "Uncaught (in promise) TypeError: Cannot read property 'sort' of undefined", source: chrome-extension://oobinhbdbiehknkpbpejbbpdbkdjmoco/remote_call.js (296)

  - selectVolume returned undefined, caller invokes .length on undefined.

Suspect is a patch landed before crrev.com/566321.  Folks, wanna re-check your patches around that time?  Do the symptoms ring any bells?

Luciano and I investigated some today.  One common thing about the tests listed above is that they all involve Drive => Drive-related changes are possible suspects.

Looking at the symptoms, the tests are complaining about not seeing the volume content they expect: the volume has bad content or has missing content.

And the failure mode seems like a race condition, eg., see test

CopyBetweenWindows/FilesAppBrowserTest.Test/copyBetweenWindowsLocalToUsb

https://logs.chromium.org/v/?s=chromium%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8943967138703573408%2F%2B%2Fsteps%2Fbrowser_tests__with_patch_%2F0%2Flogs%2FCopyBetweenWindows__x2f_FilesAppBrowserTest.Test__x2f_copyBetweenWindowsLocalToUsb%2F0

Failed three times, different failure each time, but symptoms as listed above.

So recent code changes in this area.

https://chromium.googlesource.com/chromium/src/+log/0f9ea91ea461cb3d432a250ede3b9e3ddf6b522f/chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc

And we are looking for something crrev.com/566321, so I suspect crrev.com/566319 might be a candidate.

re-assign => sashab@

Maybe we could try reverting this change?  Not sure.

@sammc  Sam, if you feel up to it, could you run your eyes over crrev.com/566319 and check that all the postTask foo and fake drive stuff is done and complete by the time the addEntries request [1] returns in the
file_manager_browsertest_base.cc?

[1] The addEntries command came came from the JS testcase, and started the whole CreateFile, CreateDirectory in Drive C++ code flow).  /me thinking that as soon as the addEntries request returns, the JS code in the test goes looking for the volume files.  It seems like they are not there sometimes.

The capabilities stuff doesn't affect volumes. I'll take a look anyway though, and own this bug.

Can we get a closer bisect? It's flaky so I know bisecting is hard.

slangley/sammc any chance this is caused by your changes to sync/DriveFS?

r566319 seems fine to me. The RunUntilIdle() stuff is pretty sus, but no more than the existing calls.

CopyBetweenWindows/FilesAppBrowserTest.Test/copyBetweenWindowsLocalToUsb doesn't involve drive so it's unlikely to be a drive-specific change.

These RPCs between the test extension and the file manager don't seem like they should ever return undefined so it's a bit strange that they are.

Agree, would like to see the RunUntilIdle() stuff improved, but good to hear that r566319 looks good.  

copyBetweenWindowsLocalToUsb: don't trust a name, Drive is used at the beginning to the test setup for later part of the test to copy from Local to USB.

I started a regression analysis from r566319, I could repro the bug in a local release build at r566319 using:

./out/Debug/browser_tests --gtest_filter="CopyBetweenWindows/FilesAppBrowserTest.Test/copyBetweenWindowsLocalToUsb" --gtest_repeat=20

It would hit the bug at that rate; about 1 repro in 20 repeats.

Started bisecting backwards, using guesswork, and found a point r566148 with no repro.  With that bisection could commence:

#566319 bad
#566230 bad
#566215 bad
#566210 bad
#566209 bad
#566208 bad
#566206 bad
#566205 good
#566200 good
#566194 good
#566154 good
#566152 good
#566148 good

crrev.com/566206, a V8 roll.

Seems it introduced a weird issue with JS Array, and I then suspected that any issue with Array be fixed pronto by the V8 team.  Moved the analysis forward toward ToT, and sure enough:

#567688 good (ToT)
#567079 good

using the repro steps.  The issue has been fixed.  I will re-enable this test and let it run over the week-end to confirm ...

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

commit e2e0878631290a294ebe2459eb2deefb5ea7a0e1
Author: Noel Gordon <noel@chromium.org>
Date: Fri Jun 15 17:41:06 2018

Re-enable CopyBetweenWindows/FilesAppBrowserTest in RELEASE

Intermittent flakes in this test was caused by a V8 roll crrev/566206.
Refer to the bug for reproduction steps and regression analysis.

Symptoms were that JS Array return types would sometimes appear in the
test code as being undefined or have no .length property, which caused
intermittent JS exceptions in the test code and hence test failure.

The issue has been fixed in V8 and no longer reproduces @ crrev/567688
ToT. Re-enabling this test RELEASE therefore <eom>.

Tbr: sashab
Bug:  852246 
Change-Id: If093f3357042e5e5dec7b399d373ba56e86dafea
Reviewed-on: https://chromium-review.googlesource.com/1102837
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567718}
[modify] https://crrev.com/e2e0878631290a294ebe2459eb2deefb5ea7a0e1/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

looking good, no evidence of flakes https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=CopyBetweenWindows%2FFilesAppBrowserTest

sammc@ > These RPCs between the test extension and the file manager don't seem like they should ever return undefined so it's a bit strange that they are.

Yes the remote called function in this case returns a JS Array, the remote_call.js calling code sometimes received 'undefined' instead of an Array, very odd.

+danno in case something needs fixing in the V8 roll to prevent bugs like this sneaking through to Chromium.

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

commit b387bc4704ed6843e1325baf03f89069a1647d99
Author: Noel Gordon <noel@chromium.org>
Date: Tue Jun 19 00:37:47 2018

Revert "Disable FolderShortcuts/FilesAppBrowserTest on ASan"

This reverts commit 210d3b47ff5bff7ca989f5126ef00c7245a7ecbc.

Reason for revert:  https://crbug.com/852246#c12 , this issue was fixed
already, no need to revert. "Slowness and times out": not so per the
logs. They were reporting JS exceptions were being thrown in test due to JS Array being 'undefined',  issue 852246 .

Original change's description:
> Disable FolderShortcuts/FilesAppBrowserTest on ASan
> 
> It's too slow and times out.
> 
> TBR=fukino@chromium.org
> 
> Bug:  851988 
> Change-Id: I868d846e3fae5f7dd16b2a2c7203602008221396
> Reviewed-on: https://chromium-review.googlesource.com/1104424
> Reviewed-by: Marc Treib <treib@chromium.org>
> Commit-Queue: Marc Treib <treib@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567991}

TBR=fukino@chromium.org,treib@chromium.org

Change-Id: Ia287519344b7aeecc03da8539fc4448e1224c740
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  851988 ,  852246 
Reviewed-on: https://chromium-review.googlesource.com/1105537
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568261}
[modify] https://crrev.com/b387bc4704ed6843e1325baf03f89069a1647d99/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

 Issue 851988  has been merged into this issue.

 Issue 852055  has been merged into this issue.

Re #19, the timeouts I was referring to weren't from the tryflakes app, but just straight from the flakiness dashboard (which was entirely unclear since I forgot to paste the link...)

Here it is:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=browser_tests&tests=FolderShortcuts/FilesAppBrowserTest.Test/addRemoveFolderShortcuts

You can see that the test typically takes around 50s on ASan and occasionally hits the 60s timeout.

Ok thanks trieb@ ... have a look at those test results as of today :)

!) Results #22 un-flaky as of today #22 FolderShortcuts/FilesAppBrowserTest*.

2) No reports of copyBetweenWindowsLocalToUsb flakes since 2018-06-12 20:51:26 UTC from chromium-try-flakes@