Avoid re-entrancy in DriveIntegrationService testing factory

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}
Committed: https://chromium.googlesource.com/chromium/src/+/fa44a170953dea4680dc68f1eb0e145b97307958

[Ken Rockot(use gerrit already), 2017-02-16 06:34:01]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 06:34:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-02-16 06:35:56]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2017-02-16 06:36:03]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-02-16 06:36:07]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-16 06:36:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-02-16 06:41:09]

Description was changed from

==========
re-enable prefs service

BUG=
==========

to

==========
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 the KeyedService factory. This behavior can trivially
lead to main-thread tasks re-entering the factory on Profile access.

Instead, we now wait until SetUpOnMainThread to populate the
fake Drive filesystem entries, because all Profiles (and their
associated DriveIntegrationService instances) are fully created
by the time SetUpOnMainThread is called, and the entries don't
need to be present until this point.

There should be no meaningful change in behavior for these tests,
but it's now safe for other parts of the browser system to run early
tasks which touch the Profile without breaking these tests.

BUG=692682
==========

[Ken Rockot(use gerrit already), 2017-02-16 06:41:50]

Description was changed from

==========
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 the KeyedService factory. This behavior can trivially
lead to main-thread tasks re-entering the factory on Profile access.

Instead, we now wait until SetUpOnMainThread to populate the
fake Drive filesystem entries, because all Profiles (and their
associated DriveIntegrationService instances) are fully created
by the time SetUpOnMainThread is called, and the entries don't
need to be present until this point.

There should be no meaningful change in behavior for these tests,
but it's now safe for other parts of the browser system to run early
tasks which touch the Profile without breaking these tests.

BUG=692682
==========

to

==========
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 the KeyedService factory. This behavior can trivially
lead to main-thread tasks re-entering the factory on Profile access.

Instead, we now wait until SetUpOnMainThread to populate the
fake Drive filesystem entries, because all Profiles and their
associated DriveIntegrationService instances are fully created
by the time SetUpOnMainThread is called, and the entries don't
need to be present until this point.

There should be no meaningful change in behavior for these tests,
but it's now safe for other parts of the browser system to run early
tasks which touch the Profile without breaking these tests.

BUG=692682
==========

[Ken Rockot(use gerrit already), 2017-02-16 06:57:00]

rockot@chromium.org changed reviewers:
+ mtomasz@chromium.org

[Ken Rockot(use gerrit already), 2017-02-16 06:57:01]

PTAL, thanks!

[mtomasz, 2017-02-16 08:48:59]

lgtm, but the tests seem to regressed

[commit-bot: I haz the power, 2017-02-16 09:23:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 09:23:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2017-02-16 17:23:37]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 17:24:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-02-16 17:26:36]

Description was changed from

==========
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 the KeyedService factory. This behavior can trivially
lead to main-thread tasks re-entering the factory on Profile access.

Instead, we now wait until SetUpOnMainThread to populate the
fake Drive filesystem entries, because all Profiles and their
associated DriveIntegrationService instances are fully created
by the time SetUpOnMainThread is called, and the entries don't
need to be present until this point.

There should be no meaningful change in behavior for these tests,
but it's now safe for other parts of the browser system to run early
tasks which touch the Profile without breaking these tests.

BUG=692682
==========

to

==========
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 the KeyedService factory. This behavior can trivially
lead to main-thread tasks re-entering the factory on Profile access.

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
==========

[Ken Rockot(use gerrit already), 2017-02-16 17:27:24]

Description was changed from

==========
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 the KeyedService factory. This behavior can trivially
lead to main-thread tasks re-entering the factory on Profile access.

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
==========

to

==========
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 the 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 the
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
==========

[Ken Rockot(use gerrit already), 2017-02-16 17:27:57]

Description was changed from

==========
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 the 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 the
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
==========

to

==========
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
==========

[Ken Rockot(use gerrit already), 2017-02-16 17:30:08]

Thanks for the review. Since I've completely changed the approach here to
something I feel is much less fragile, I'd like to solicit you for one more
pass. :)

[Ken Rockot(use gerrit already), 2017-02-16 17:31:45]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 17:32:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-02-16 17:32:21]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-16 18:08:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 18:08:16]

Dry run: This issue passed the CQ dry run.

[mtomasz, 2017-02-17 02:53:53]

https://codereview.chromium.org/2695403002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc (left):

https://codereview.chromium.org/2695403002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:214: if
(error != google_apis::HTTP_CREATED)
We're losing all these error checks, right? This sounds like unnecessary
regression. Why didn't your previous approach work?

[Ken Rockot(use gerrit already), 2017-02-17 02:56:25]

On Feb 16, 2017 6:53 PM, <mtomasz@chromium.org> wrote:


https://codereview.chromium.org/2695403002/diff/80001/
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
File chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc
(left):

https://codereview.chromium.org/2695403002/diff/80001/
chrome/browser/chromeos/file_manager/external_filesystem_
apitest.cc#oldcode214
chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc:214:
if (error != google_apis::HTTP_CREATED)
We're losing all these error checks, right? This sounds like unnecessary
regression. Why didn't your previous approach work?


Work can be done in the background by component extension background pages
and some tests rely on it being affected by the filesystem state.

The error checks are pointless as this is a fake object and the calls can
never fail.


https://codereview.chromium.org/2695403002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mtomasz, 2017-02-17 03:00:43]

sgtm. lgtm, thanks for fixing!

[mtomasz, 2017-02-17 03:00:43]

sgtm. lgtm, thanks for fixing!

[Ken Rockot(use gerrit already), 2017-02-17 03:06:49]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2017-02-17 03:07:39]

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, 2017-02-17 04:49:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487300809039580,
"parent_rev": "ec269885f32e22ded6c3d0531038a18782e5c438", "commit_rev":
"fa44a170953dea4680dc68f1eb0e145b97307958"}

[commit-bot: I haz the power, 2017-02-17 04:49:44]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa44a170953dea4680dc68f1eb0e...
==========

[commit-bot: I haz the power, 2017-02-17 04:49:45]

Committed patchset #2 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/fa44a170953dea4680dc68f1eb0e...