Expose mock geofencing service via testRunner.

Expose mock geofencing service via testRunner.

BUG=383125
Committed: https://crrev.com/340dd9d3de95a7e7c027972cce7168f201ae84e8
Cr-Commit-Position: refs/heads/master@{#307836}

[Marijn Kruisselbrink, 2014-12-03 14:20:18]

Patchset #5 (id:80001) has been deleted

[Marijn Kruisselbrink, 2014-12-03 14:20:32]

Patchset #4 (id:60001) has been deleted

[Marijn Kruisselbrink, 2014-12-03 14:20:40]

Patchset #3 (id:40001) has been deleted

[Marijn Kruisselbrink, 2014-12-03 14:42:37]

Patchset #3 (id:100001) has been deleted

[Marijn Kruisselbrink, 2014-12-03 15:07:21]

mek@chromium.org changed reviewers:
+ scheib@chromium.org

[Marijn Kruisselbrink, 2014-12-03 15:07:22]

Does this seem like a reasonable approach to making the geofencing code testable
in layout tests? Ideally there would of course be some standardized location
mocking API, but for now this seems like it would work well.

The (in progress) blink side of this lives in
https://codereview.chromium.org/702983002/

[Peter Beverloo, 2014-12-03 15:23:08]

On 2014/12/03 15:07:22, Marijn Kruisselbrink wrote:
> Does this seem like a reasonable approach to making the geofencing code
testable
> in layout tests? Ideally there would of course be some standardized location
> mocking API, but for now this seems like it would work well.
> 
> The (in progress) blink side of this lives in
> https://codereview.chromium.org/702983002/

Check out //content/shell/browser/layout_test/, which might be helpful here.

[scheib, 2014-12-03 17:54:31]

https://codereview.chromium.org/701953007/diff/120001/content/browser/geofenc...
File content/browser/geofencing/geofencing_manager.h (right):

https://codereview.chromium.org/701953007/diff/120001/content/browser/geofenc...
content/browser/geofencing/geofencing_manager.h:188: std::map<int64,
linked_ptr<MockGeofencingService>> mock_services_;
Why not have the mock simply replace service_? Why have a mock per worker?

[Marijn Kruisselbrink, 2014-12-04 13:44:11]

On 2014/12/03 17:54:31, scheib wrote:
>
https://codereview.chromium.org/701953007/diff/120001/content/browser/geofenc...
> File content/browser/geofencing/geofencing_manager.h (right):
> 
>
https://codereview.chromium.org/701953007/diff/120001/content/browser/geofenc...
> content/browser/geofencing/geofencing_manager.h:188: std::map<int64,
> linked_ptr<MockGeofencingService>> mock_services_;
> Why not have the mock simply replace service_? Why have a mock per worker?
I've been going back and forth on this. On the one hand one global mock would
make the code a lot simpler, but it would make actual tests a lot more
complicated. By default multiple asynchronous testharness based tests in a
single file can run interleaved, which would not work with a global mock, so I'd
be limited to a single test case per file. With separate mocks for specific
service worker registrations this is no longer a problem.

Another reason against global mocks is what should happen with existing geofence
registrations when you enable mocking. When mocking is at a SW registration
level just unregistering all geofences for that service worker when mocking is
enabled/disabled seems like a pretty sane and safe thing to do, but to do the
same for all service workers would be a bit more invasive. Moving existing
registrations over to the new service is also complicated since either the mock
service or the real service might not allow geofences to be registered.

Ideally of course this would also eventually be hooked up to the devtools and
chromedriver exposed mock geolocation, but that makes things even more
complicated. Currently that mock geolocation seems to be global per renderer
process (although both the devtools and chromedriver access points look like it
should be per document). So I'm not sure yet what this unified mock geolocation
will look like (and maybe I'll have to separate the mock position from the mock
geofence availability), but having a single global (or more precise, per storage
partition) mock geolocation and "is geofencing available" flag seems like it
would make writing tests pretty hard (but not impossible).

I think patch set 2 mostly implemented the mock geofencing per storage partition
option, so if you prefer I go back to that, that might make sense until we
figure out what a standardized geofencing mocking API would look like, and how
this could be integrated with the existing devtools and chromedriver exposed
mock geolocation.

[scheib, 2014-12-05 05:56:04]

On 2014/12/04 13:44:11, Marijn Kruisselbrink wrote:
> On 2014/12/03 17:54:31, scheib wrote:
> >
>
https://codereview.chromium.org/701953007/diff/120001/content/browser/geofenc...
> > File content/browser/geofencing/geofencing_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/701953007/diff/120001/content/browser/geofenc...
> > content/browser/geofencing/geofencing_manager.h:188: std::map<int64,
> > linked_ptr<MockGeofencingService>> mock_services_;
> > Why not have the mock simply replace service_? Why have a mock per worker?
> I've been going back and forth on this. On the one hand one global mock would
> make the code a lot simpler, but it would make actual tests a lot more
> complicated. By default multiple asynchronous testharness based tests in a
> single file can run interleaved, which would not work with a global mock, so
I'd
> be limited to a single test case per file. With separate mocks for specific
> service worker registrations this is no longer a problem.
> 
> Another reason against global mocks is what should happen with existing
geofence
> registrations when you enable mocking. When mocking is at a SW registration
> level just unregistering all geofences for that service worker when mocking is
> enabled/disabled seems like a pretty sane and safe thing to do, but to do the
> same for all service workers would be a bit more invasive. Moving existing
> registrations over to the new service is also complicated since either the
mock
> service or the real service might not allow geofences to be registered.
> 
> Ideally of course this would also eventually be hooked up to the devtools and
> chromedriver exposed mock geolocation, but that makes things even more
> complicated. Currently that mock geolocation seems to be global per renderer
> process (although both the devtools and chromedriver access points look like
it
> should be per document). So I'm not sure yet what this unified mock
geolocation
> will look like (and maybe I'll have to separate the mock position from the
mock
> geofence availability), but having a single global (or more precise, per
storage
> partition) mock geolocation and "is geofencing available" flag seems like it
> would make writing tests pretty hard (but not impossible).
> 
> I think patch set 2 mostly implemented the mock geofencing per storage
partition
> option, so if you prefer I go back to that, that might make sense until we
> figure out what a standardized geofencing mocking API would look like, and how
> this could be integrated with the existing devtools and chromedriver exposed
> mock geolocation.

This discussion seems reasonable in explaining why not global -- it just wasn't
obvious when reading over code. Include a section in comments (file level? class
level?) covering some of these issues.

(Repeating what we discussed in chat:)
Platform internals should be injected via testRunner, see review comments and
final patch here for the same issue I recently went through with bluetooth:
 https://codereview.chromium.org/650613005/

[Marijn Kruisselbrink, 2014-12-07 23:52:15]

Patchset #4 (id:140001) has been deleted

[Marijn Kruisselbrink, 2014-12-08 00:00:58]

On 2014/12/05 05:56:04, scheib wrote:
> This discussion seems reasonable in explaining why not global -- it just
wasn't
> obvious when reading over code. Include a section in comments (file level?
class
> level?) covering some of these issues.
Actually I now ended up going back to global anyway... Somehow trying to pass
some way to identify a service worker registration to a testRunner exposed
method made this way more complicated, and for now just having a global mock
should be good enough.

> (Repeating what we discussed in chat:)
> Platform internals should be injected via testRunner, see review comments and
> final patch here for the same issue I recently went through with bluetooth:
>  https://codereview.chromium.org/650613005/
Done, PTAL

[scheib, 2014-12-08 17:27:08]

lgtm

[Marijn Kruisselbrink, 2014-12-08 19:23:13]

mek@chromium.org changed reviewers:
+ avi@chromium.org

[Marijn Kruisselbrink, 2014-12-08 19:23:14]

+avi for content/ OWNERS
let me know if I should add a separate content/shell owner (in which case
content/common, content/public and content/test are the only other bits of
content/ that I'd need you to review)

[Avi (use Gerrit), 2014-12-08 19:33:05]

I would appreciate if you could find a content shell owner. I wrote the UI
parts, but am a bit fuzzy on the testing cases.

It's weird that the renderer can ask the browser for a mock geofencing provider.
Can we restrict that to testing code only?

[Marijn Kruisselbrink, 2014-12-08 19:51:54]

mek@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Marijn Kruisselbrink, 2014-12-08 19:51:55]

On 2014/12/08 19:33:05, Avi wrote:
> I would appreciate if you could find a content shell owner. I wrote the UI
> parts, but am a bit fuzzy on the testing cases.
+pfeldman for content/shell OWNERS

> It's weird that the renderer can ask the browser for a mock geofencing
provider.
> Can we restrict that to testing code only?
I could of course move the IPC to this to content shell, like some of the other
testRunner exposed APIs, although ideally this mock wouldn't be restricted to
testing code only at all. Ideally this would be unified with the existing mock
geolocation that is already exposed via devtools and chromedriver (but figuring
out the right way to do that is beyond the scope of this CL).
And maybe some day mocking things like this would even be a standardized web
API, to make it better possible to have cross browser automated tests for
features like this (although probably for such an API the mocking should be
scoped to only influence geofences for one origin (or service worker
registration)).

[Avi (use Gerrit), 2014-12-08 20:21:54]

On 2014/12/08 19:51:55, Marijn Kruisselbrink wrote:
> On 2014/12/08 19:33:05, Avi wrote:
> > I would appreciate if you could find a content shell owner. I wrote the UI
> > parts, but am a bit fuzzy on the testing cases.
> +pfeldman for content/shell OWNERS
> 
> > It's weird that the renderer can ask the browser for a mock geofencing
> provider.
> > Can we restrict that to testing code only?
> I could of course move the IPC to this to content shell, like some of the
other
> testRunner exposed APIs, although ideally this mock wouldn't be restricted to
> testing code only at all. Ideally this would be unified with the existing mock
> geolocation that is already exposed via devtools and chromedriver (but
figuring
> out the right way to do that is beyond the scope of this CL).
> And maybe some day mocking things like this would even be a standardized web
> API, to make it better possible to have cross browser automated tests for
> features like this (although probably for such an API the mocking should be
> scoped to only influence geofences for one origin (or service worker
> registration)).

There's a parallel geolocation mock for devtools? ⊙_◎

[pfeldman, 2014-12-10 08:59:52]

lgtm

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.h:517: // Disables mock
geofencing service while running a layout test.
Blank line above the comment please.

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
File content/shell/renderer/test_runner/web_test_delegate.h (right):

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
content/shell/renderer/test_runner/web_test_delegate.h:160: // Disables mock
geofencing service while running a layout test.
ditto

https://codereview.chromium.org/701953007/diff/160001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/701953007/diff/160001/content/test/layouttest...
content/test/layouttest_support.cc:325: ->SetMockProvider(service_available);
wrong indentation.

[Marijn Kruisselbrink, 2014-12-10 17:51:44]

Patchset #5 (id:180001) has been deleted

[Marijn Kruisselbrink, 2014-12-10 18:13:37]

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_runner.h (right):

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
content/shell/renderer/test_runner/test_runner.h:517: // Disables mock
geofencing service while running a layout test.
On 2014/12/10 08:59:52, pfeldman wrote:
> Blank line above the comment please.

Done.

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
File content/shell/renderer/test_runner/web_test_delegate.h (right):

https://codereview.chromium.org/701953007/diff/160001/content/shell/renderer/...
content/shell/renderer/test_runner/web_test_delegate.h:160: // Disables mock
geofencing service while running a layout test.
On 2014/12/10 08:59:52, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/701953007/diff/160001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/701953007/diff/160001/content/test/layouttest...
content/test/layouttest_support.cc:325: ->SetMockProvider(service_available);
On 2014/12/10 08:59:52, pfeldman wrote:
> wrong indentation.
this was the indentation clang-format preferred, so I'm not entirely sure what
you consider the "correct" indentation in this particular case, but I'm guessing
an extra 4 spaces?

[Marijn Kruisselbrink, 2014-12-10 18:16:22]

mek@chromium.org changed reviewers:
+ tsepez@chromium.org

[Marijn Kruisselbrink, 2014-12-10 18:16:23]

+tsepez for IPC OWNERS

avi: could you PTAL for content/common, content/public and content/test OWNERS

[Tom Sepez, 2014-12-10 19:17:51]

On 2014/12/10 18:16:23, Marijn Kruisselbrink wrote:
> +tsepez for IPC OWNERS
So it looks like this code is included in a production chrome binary, which is
unfortunate mainly due to bloat.  Is there a way around this? 

Also, a compromised renderer/SW could send this message.  However, it looks like
only that child process would be affected (i.e. since it's non-global, it
couldn't DoS other renderer/SW).  Please confirm.

[Avi (use Gerrit), 2014-12-10 19:32:14]

I don't have a problem with it from the content/ standpoint. It's bothering me
that we have two mocking mechanisms here: one for tests, and one for devtools.
Can we combine them? Can we at least have a bug to combine them?

[Marijn Kruisselbrink, 2014-12-10 19:45:06]

On 2014/12/10 19:17:51, Tom Sepez wrote:
> On 2014/12/10 18:16:23, Marijn Kruisselbrink wrote:
> > +tsepez for IPC OWNERS
> So it looks like this code is included in a production chrome binary, which is
> unfortunate mainly due to bloat.  Is there a way around this? 
One day I hope to unify this geofence mock location with the existing
geolocation
mock location, so while currently this is only exposed to layout tests, that
will
hopefully change once I figure out a way to do so. So yes, I could move most of
this
code to only exist in content_shell, but I'd rather not.

> Also, a compromised renderer/SW could send this message.  However, it looks
like
> only that child process would be affected (i.e. since it's non-global, it
> couldn't DoS other renderer/SW).  Please confirm.
This is not true. The geofencing mock as in the current version of this CL is
global.
It affects all geofences associated with any service worker registration. I
started
out this CL with a global mock, then switched to a
per-service-worker-registration
mock since that seemed more secure and would make using the mock easier, and
then
in the end I went back to a global mock, because passing a service worker
registration
to a testRunner exposed API is rather non-trivial. A mock per origin might be a
sane
middle ground, although it wouldn't quite help against a compromised renderer,
since
the browser process would have no way of knowing if a renderer is allowed to
enabling
mocking for a specific origin (or is there a way to do that?).

I could of course move the entire IPC to content_shell, which would solve these
security
issues in a production chrome binary, but ideally in the future this would be
unified with
the existing mock geolocation support that is currently exposed in devtools and
chromedriver, at which point some kind of IPC like this is going to be necesary
again.
It's unfortunate that due to the nature of service workers (where the worker and
its
clients don't all live in the same renderer process) it isn't possible to keep
geofence
mocking scoped to a renderer process like the existing geolocation mocking is.
So I'm not
sure what the best answer here is/what an acceptable level of risk is in the
presence of
compromised renderers (a compromised renderer can already do basically anything
with the
geofence registrations of any arbitrary service worker, and when I explicitly
asked about
that I was told that was not a problem to be worried about).

[Marijn Kruisselbrink, 2014-12-10 19:45:41]

On 2014/12/10 19:32:14, Avi wrote:
> I don't have a problem with it from the content/ standpoint. It's bothering me
> that we have two mocking mechanisms here: one for tests, and one for devtools.
> Can we combine them? Can we at least have a bug to combine them?
Well, we have two mocking mechanisms, one for devtools/chromedriver for
geolocation,
and one for tests for geofencing. And yes, having two separate mock geolocation
for these two features is confusing, and is something I want to resolve. I'm not
entirely sure yet what the shape of such a combined mock will look like as it
will require changing the behavior of the geolocation mock slightly too (the
geolocation mock is currently scoped per renderer process, but service workers
and geofences can live outside a renderer process).
I'll file a bug to unify these two.

[Marijn Kruisselbrink, 2014-12-10 19:52:48]

On 2014/12/10 19:45:41, Marijn Kruisselbrink wrote:
> On 2014/12/10 19:32:14, Avi wrote:
> > I don't have a problem with it from the content/ standpoint. It's bothering
me
> > that we have two mocking mechanisms here: one for tests, and one for
devtools.
> > Can we combine them? Can we at least have a bug to combine them?
> Well, we have two mocking mechanisms, one for devtools/chromedriver for
> geolocation,
> and one for tests for geofencing. And yes, having two separate mock
geolocation
> for these two features is confusing, and is something I want to resolve. I'm
not
> entirely sure yet what the shape of such a combined mock will look like as it
> will require changing the behavior of the geolocation mock slightly too (the
> geolocation mock is currently scoped per renderer process, but service workers
> and geofences can live outside a renderer process).
> I'll file a bug to unify these two.
Filed http://crbug.com/440902

[Avi (use Gerrit), 2014-12-10 19:55:02]

On 2014/12/10 19:52:48, Marijn Kruisselbrink wrote:
> On 2014/12/10 19:45:41, Marijn Kruisselbrink wrote:
> > On 2014/12/10 19:32:14, Avi wrote:
> > > I don't have a problem with it from the content/ standpoint. It's
bothering
> me
> > > that we have two mocking mechanisms here: one for tests, and one for
> devtools.
> > > Can we combine them? Can we at least have a bug to combine them?
> > Well, we have two mocking mechanisms, one for devtools/chromedriver for
> > geolocation,
> > and one for tests for geofencing. And yes, having two separate mock
> geolocation
> > for these two features is confusing, and is something I want to resolve. I'm
> not
> > entirely sure yet what the shape of such a combined mock will look like as
it
> > will require changing the behavior of the geolocation mock slightly too (the
> > geolocation mock is currently scoped per renderer process, but service
workers
> > and geofences can live outside a renderer process).
> > I'll file a bug to unify these two.
> Filed http://crbug.com/440902

LGTM if you link that bug in the code.

[Tom Sepez, 2014-12-10 20:01:09]

It's not ideal, but I'm not going to hold this up for compromised renderers
(since there's worse that can happen).  LGTM on messages.

[Marijn Kruisselbrink, 2014-12-11 01:01:14]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-12-11 01:03:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701953007/220001

[commit-bot: I haz the power, 2014-12-11 02:11:12]

Committed patchset #6 (id:220001)

[commit-bot: I haz the power, 2014-12-11 02:11:55]

Patchset 6 (id:??) landed as
https://crrev.com/340dd9d3de95a7e7c027972cce7168f201ae84e8
Cr-Commit-Position: refs/heads/master@{#307836}