New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 692682 link

Starred by 1 user
Status: Fixed
Owner:
█ roc...@chromium.org
please use my google.com address
Closed: Feb 2017
Cc:
█ hirono@chromium.org
kinaba@chromium.org
tbarzic@chromium.org
█ mtomasko@chromium.org
jonr...@chromium.org
hashimoto@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Preferences service breaks filesystem API tests on Chrome OS

Project Member Reported by roc...@chromium.org, Feb 15 2017 
Test fixtures in //chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc and //chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc initialize the DriveIntegrationService keyed service via a factory which actually flushes the main thread message loop before returning the service instance.

Attempting to access the active Profile on Chrome OS will ultimately require the existence of a DriveIntegrationService for the BrowserContext, and so if one has not yet been not set, the factory will be invoked to create one.

This is a problem because a task may be be posted to the main thread task runner which accesses the Profile, and that task may now be run within the stack frame of the test's DriveIntegrationService factory as it's spinning the loop. This means the factory function becomes unexpectedly re-entrant.

I uncovered this bug while attempting to make the preferences service reachable once again. Ash connects to the service early during startup, and the preferences service wants to monitor the active Profile for shutdown once it has a non-zero number of clients. The net result is that in filesystem API tests, the DriveIntegrationService testing factory is re-entered within itself and things explode.

It seems clear to me that it's generally unsafe to be spinning the message loop within a KeyedService factory function since this is essentially a facility for global dependency injection that is not designed to be re-entrant. So I think we need to find a way to avoid such behavior in these tests.

CCing folks who know about either prefs service or (at least probably, based on history) filesystem API tests.

Comment 1 by jonr...@chromium.org, Feb 15 2017 

I'm not sure on the requirements of the filesystem api tests. In general I try to avoid pumping the message loop unless testing functions which post tasks.

From the preferences side, it appears that the new service may have moved up the access of a profile earlier than before, exposing this. There are several areas in ash which have always accessed preferences, they likely just happened to be running late enough to have not triggered this conflict before.

Comment 2 by roc...@chromium.org, Feb 15 2017 

Yeah I'm sure my recent change to service packaging has tickled this bug,
but I avoided hitting it with my CL by deferring the lazy initialization of
the profile shutdown observer until the pref service is actually used. It
doesn't actually get used right now because of the incorrect manifest
entry, but I obviously want to fix that.

I'm going to attempt to change the behavior of these tests.

Comment 3 by hashimoto@chromium.org, Feb 16 2017

Cc: hirono@chromium.org mtomasko@chromium.org kinaba@chromium.org
Adding people who should be more familiar with external_filesystem_apitest.cc and file_system_apitest_chromeos.cc than me.

I think we can avoid spinning the message loop in DriveIntegrationService factory by either:
 1. Using FakeDriveService::AddNewFileWithResourceId/AddNewDirectoryWithResourceId to construct the fake data, and omitting error check for these methods.
 2. Do the fake data setup in a different place (e.g. in SetUpOnMainThread()).

#2 is easier than #1, but it may affect the behavior of the tests.

Comment 4 by roc...@chromium.org, Feb 16 2017 

Thanks for commenting! I put up a CL last night which does #2, but that seems to break a few tests due to the timing of component extension loading.

#1 sounds like the better approach to me. Upon further inspection I agree that there's no reason why FakeDriveService setup has to be done asynchronously here.

I've taken this approach now in https://codereview.chromium.org/2695403002.
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 17 2017 

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

commit fa44a170953dea4680dc68f1eb0e145b97307958
Author: rockot <rockot@chromium.org>
Date: Fri Feb 17 04:49:02 2017

Avoid re-entrancy in DriveIntegrationService testing factory

This avoids potential re-entrancy in various Drive API integration
tests which was made possible by running the MessageLoop from within
the stack of a KeyedService factory. This behavior can trivially
lead to main-thread tasks re-entering the factory if they try to
access a Profile, as Profile access on Chrome OS lazily invokes this
factory.

With minimal modifications, we avoid the need to wait for any async
work in FakeDriveService and thus avoid the need to run the MessageLoop
within the test service factory.

There's also some minor cleanup here: NULL -> nullptr, and switched to
field initializers where appropriate.

BUG= 692682 

Review-Url: https://codereview.chromium.org/2695403002
Cr-Commit-Position: refs/heads/master@{#451220}

[modify] https://crrev.com/fa44a170953dea4680dc68f1eb0e145b97307958/chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
[modify] https://crrev.com/fa44a170953dea4680dc68f1eb0e145b97307958/chrome/browser/extensions/api/file_system/file_system_apitest_chromeos.cc

Comment 6 by roc...@chromium.org, Feb 17 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment