New issue
Advanced search Search tips

Issue 895703 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 836254
issue 879434
issue 453634



Sign in to add a comment

Re-enable select_file_dialog_extension_browsertest suite

Project Member Reported by noel@chromium.org, Oct 16

Issue description

Fixing Files app extension dialog  issue 453634 , noticed [1] the dialog extension browser tests

  chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

are disabled on Chrome OS.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1280383#message-3369b43a9740e06f852a4c8e44a0d45b117d1b3d

We should work out why the tests are failing.  The tests were blanket disabled in code revision:

https://chromium.googlesource.com/chromium/src/+/24e0842a2a9d8f5aaacabc4a3ea94b3c22995e31%5E%21/#F0

under bug  issue 477360 , then that issue was archived.

Related bugs:

Use "ready" message instead of "worker-initialized" message for test
 -- details of how the messages work.
https://bugs.chromium.org/p/chromium/issues/detail?id=432029

SelectFileDialogExtensionBrowserTest.SelectFileAndCancel flaky on CrOS MSAN
 -- has a trace and fix for a different kind of flake
https://bugs.chromium.org/p/chromium/issues/detail?id=453613

 
Cc: slangley@chromium.org
Labels: CrOSFilesCategory-Testing OS-Chrome
Status: Available (was: Untriaged)
"under bug  issue 477360 , then that issue was archived", that issue had a crash stack, and in it is a fatal error.

[19446:19446:0415/082737:FATAL:private_api_file_system.cc(687)] Check failed: external_backend->CanHandleType(file_system_url.type()).
#0 0x7fd32947c17e base::debug::StackTrace::StackTrace()
#1 0x7fd3294d2cbf logging::LogMessage::~LogMessage()

Now I wonder where we've seen that before (no prizes for guessing).
Cc: benwells@chromium.org
Answ: https://bugs.chromium.org/p/chromium/issues/detail?id=804413 ... which was one symptom of the great File Manager Browser Test flakiness problem:

    issue 831074   issue 804413   issue 829310 

Now we see  issue 477360  as evidence that that test flakiness problem existed as far back as Apr 15, 2015.
Blocking: 836254
Blocking: 453634
The systemic flake in these tests is caused by the same issue identified and fixed the File Manager browser tests:  crbug.com/831074#c11 

 "If you want to use a resource during testing, then ensuring that resource is
  ready, before the app under test starts and looks for it, and before testing
  even begins, is the well-known recipe for success".

Without that, systemic flakiness results. These browser tests in the SetUp() phase call:

  extensions::ComponentLoader::EnableBackgroundExtensionsForTesting();

which allows the FilesApp extensions to start, but only later do they setup the needed "Downloads" mount point resource FilesApp will look for.

The causes a race. If FilesApp wins the race, it tries to access "Downloads", which is not yet setup, and hits a CHECK stop crash

[19446:19446:0415/082737:FATAL:private_api_file_system.cc(687)] Check failed: external_backend->CanHandleType(file_system_url.type()).
#0 0x7fd32947c17e base::debug::StackTrace::StackTrace()
#1 0x7fd3294d2cbf logging::LogMessage::~LogMessage()

Status: Started (was: Available)
Owner: noel@chromium.org
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 17

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

commit 684ff899d4dc1152888bdbf5604859b5d2898d73
Author: Noel Gordon <noel@chromium.org>
Date: Wed Oct 17 03:45:05 2018

Deflake the SelectFileDialogExtensionBrowsertest setup code

Setup the "Downloads" directory in a SetUpOnMainThread step, where the
required profile() first becomes available.

"Downloads" is a required resource for FilesApp so enable the FilesApp
component extensions _after_ it has been created. Doing the reverse as
in the original code caused a data race accessing "Downloads", and the
result was systemic test flakiness ( issue 477360 ).

Other code health and clean-up:

 - remove AddMountPoint()
     not needed anymore: "Downloads" setup is now handled by the
     SetUpOnMainThread step.
 - tune code comments
     make them precise and up-to-date: 'selection-change-complete'
     was mentioned, but was removed in later refactoring CLs.
 - add future TODO
     need to fix a FilesApp issue with CheckJavascriptErrors.
 - base::ScopedTempDir tmp_dir_
     make it the first member of the helper class, so it's dtor()
     is called last. Life on the bots is more robust that way.

No change in behavior, no new tests: disabled tests will be re-enabled
in follow-up changes (so the chrome sheriff has an easier job).

Bug:  895703 
Change-Id: Ib75ee7163ecc3aec62c474f9834ab839ae879dfd
Reviewed-on: https://chromium-review.googlesource.com/c/1285830
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600267}
[modify] https://crrev.com/684ff899d4dc1152888bdbf5604859b5d2898d73/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 17

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

commit bfc307720e6bdb2607ddd59a1dc04abadfd49e39
Author: Noel Gordon <noel@chromium.org>
Date: Wed Oct 17 05:25:21 2018

Re-enable SelectFileDialogExtensionBrowserTest/SelectFileAndCancel

Bug:  895703 
Change-Id: I529f7a310c4323cc5a3d87a978e9cbcfe0049fe2
Reviewed-on: https://chromium-review.googlesource.com/c/1285491
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600287}
[modify] https://crrev.com/bfc307720e6bdb2607ddd59a1dc04abadfd49e39/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 17

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

commit ee6a4863d99cdffbb2e90ddee81982ab2b8ac69e
Author: Noel Gordon <noel@chromium.org>
Date: Wed Oct 17 05:28:28 2018

Re-enable SelectFileDialogExtensionBrowserTest/SelectFileAndOpen

No-presubmit: true
Bug:  895703 
Change-Id: I9c56865d92ecf67292c8adc4d41d72b1d5ae1aec
Reviewed-on: https://chromium-review.googlesource.com/c/1285869
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600288}
[modify] https://crrev.com/ee6a4863d99cdffbb2e90ddee81982ab2b8ac69e/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 17

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

commit b905d48009cdaebc6bf69d605c7e8778666b5aeb
Author: Noel Gordon <noel@chromium.org>
Date: Wed Oct 17 05:31:56 2018

Re-enable SelectFileDialogExtensionBrowserTest/SelectFileAndSave

No-presubmit: true
Bug:  895703 
Change-Id: Ib7996d7f75e2c7bda08bf7ac01b45288e8fb022a
Reviewed-on: https://chromium-review.googlesource.com/c/1285889
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600290}
[modify] https://crrev.com/b905d48009cdaebc6bf69d605c7e8778666b5aeb/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 17

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

commit 025b8fc8b1d07731337e3d7915b7d11c0b758ed1
Author: Noel Gordon <noel@chromium.org>
Date: Wed Oct 17 06:27:23 2018

Re-enable SelectFileDialogExtensionBrowserTest/OpenSingletonTabAndCancel

No-presubmit: true
Bug:  895703 
Change-Id: If777a1a20b66411d99a5655b72c9a1b957670201
Reviewed-on: https://chromium-review.googlesource.com/c/1285909
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600300}
[modify] https://crrev.com/025b8fc8b1d07731337e3d7915b7d11c0b758ed1/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 17

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

commit 8eed5bfe62aef636d8c5cd1c16603b5957002d72
Author: Noel Gordon <noel@chromium.org>
Date: Wed Oct 17 06:28:53 2018

Re-enable SelectFileDialogExtensionBrowserTest/OpenTwoDialogs

No-presubmit: true
Bug:  895703 
Change-Id: Ia8322806f4c2b584536065f35429466ab0a22ba4
Reviewed-on: https://chromium-review.googlesource.com/c/1285254
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600301}
[modify] https://crrev.com/8eed5bfe62aef636d8c5cd1c16603b5957002d72/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

Status: Fixed (was: Started)
All tests green on all bots [1], no flakes.

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=SelectFileDialogExtensionBrowserTest



SelectFileDialogExtensionBrowserTest.18-10-18.png
297 KB View Download
Blocking: 879434
Status: Verified (was: Fixed)
With this suite re-enabled, we now have another test group that can help address issue 879434, being the complete lack of ash virtual keyboard tests in FilesApp.

Bots #15 looking all-green today as well: verified fixed.



Cc: sa...@chromium.org
Status: Started (was: Verified)
Re-opening to land a TODO fix.  Test opening FilesApp file dialog from a web page.
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 13

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

commit 84188d7c02dcc31d16fc32e55bf9a3336576ea2f
Author: Noel Gordon <noel@chromium.org>
Date: Tue Nov 13 03:26:55 2018

Test opening the extension dialog from web page file <input> element

FilesApp provides the innards of the extension file dialog. Add a test
to open the file dialog by clicking on an <input type=file> element in
a web page.

Bug:  895703 
Change-Id: Id6f56e2df4034af66a57f888195bc841a55da2b4
Reviewed-on: https://chromium-review.googlesource.com/c/1331099
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607470}
[modify] https://crrev.com/84188d7c02dcc31d16fc32e55bf9a3336576ea2f/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
[add] https://crrev.com/84188d7c02dcc31d16fc32e55bf9a3336576ea2f/chrome/test/data/chromeos/file_manager/file_input/element.html

Status: Fixed (was: Started)
Cc: yusukes@chromium.org
Status: Started (was: Fixed)
Project Member

Comment 22 by bugdroid1@chromium.org, Nov 15

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

commit 8b764625cfe5f18e10b3e5bca0267fdcb0751031
Author: Noel Gordon <noel@chromium.org>
Date: Thu Nov 15 01:49:22 2018

Add FilesApp owners for select_file_dialog_extension

FilesApp OWNERS should be consulted, and also help review changes, to
the ChromeOS system modal select file dialog.

Bug:  895703 
Change-Id: I4258de9c2b3bfc7e37e68b546c9e16a1395572dc
Reviewed-on: https://chromium-review.googlesource.com/c/1336952
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608216}
[modify] https://crrev.com/8b764625cfe5f18e10b3e5bca0267fdcb0751031/chrome/browser/ui/views/OWNERS

Status: Fixed (was: Started)
Status: Started (was: Fixed)
Project Member

Comment 25 by bugdroid1@chromium.org, Dec 5

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

commit 5ffc039907a8f6d9bb8db8b072d6c8d45c208ca0
Author: Noel Gordon <noel@chromium.org>
Date: Wed Dec 05 03:28:24 2018

select_file_dialog_extension_browsertest: assert owning_window

ASSERT owning_window state in each test case. The recently added test,
see CL:1337144, added a new test case for when there is no window that
owns the dialog (good). The test was restricted to CHROME_OS, but this
entire browsertest suite is CHROME_OS only, so no need to restrict.

Test: --gtest_filter="SelectFileDialogExtensionBrowserTest*"
Tbr: lucmult-away@
Bug:  895703 
Change-Id: I9b201505e0ced91c96885f5d1868a4a98e1b2007
Reviewed-on: https://chromium-review.googlesource.com/c/1361756
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613847}
[modify] https://crrev.com/5ffc039907a8f6d9bb8db8b072d6c8d45c208ca0/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc

Status: Fixed (was: Started)

Sign in to add a comment