Support for shared invalidator client IDs

Support for shared invalidator client IDs

Allow the Java side of the Andorid invalidations implementation to share
its client ID with its associated C++ classes.

The InvalidationController class will prefer to use an ID from a provided
UniqueIdentificationGeneratorFactory.  This ID would have the advantage of
persisting across restarts, which is necessary to ensure effective reflection
blocking.

If the UniqueIdentificationGeneratorFactory has not been provided, as may be
the case in tests, the InvalidationController will generate its own temporary
ID.  This will effectievely disable reflection blocking, but it's better than
returning a consistent ID (since that would risk blocking notifications that we
actually need).  This randomly generated ID is not much different from the
randomly generated ID created by the InvalidationService prior to this CL.

At the time of this writing, the code to set up the
UniqueIDGeneratorFactory is not in place.  This will be added in a later
commit.  Until then, reflection blocking will remain broken.

BUG=172391
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234524

[rlarocque, 2013-10-31 21:57:10]

Here's the next patch towards a consistent invalidation ID.

If there is no registered UniqueIdentificationGenerator for
InvalidationController.GENERATOR_ID, then the InvalidationController falls back
to generating its own ID.  Unlike the current system, it will share this ID with
the C++ side of the code.  I think the effects will be almost the same as what
happens today, where the InvalidationService generates a random ID and doesn't
share it.

I'll add code to instantiate and register a UniqueIdentificationGenerator for
InvalidationController.GENERATOR_ID in a future CL.

[nyquist, 2013-11-01 16:21:01]

https://codereview.chromium.org/54923003/diff/30001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(right):

https://codereview.chromium.org/54923003/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:167:
} catch (Exception e) {
The declaration doesn't have an exception, so you should make it clear what you
are interested in. Change to RuntimeException.

https://codereview.chromium.org/54923003/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:168:
// Tests won't have a generator available.  We need to handle
Native tests?

https://codereview.chromium.org/54923003/diff/30001/sync/android/java/src/org...
File sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
(right):

https://codereview.chromium.org/54923003/diff/30001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java:94: //
Any intents sent to the InvalidationService should include the
You have 100 characters to work with...

[rlarocque, 2013-11-01 17:24:21]

PTAL.  

Feel free to contact me on IM if you want to discuss the testing issue.

https://codereview.chromium.org/54923003/diff/30001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(right):

https://codereview.chromium.org/54923003/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:167:
} catch (Exception e) {
On 2013/11/01 16:21:01, nyquist wrote:
> The declaration doesn't have an exception, so you should make it clear what
you
> are interested in. Change to RuntimeException.

Done.

https://codereview.chromium.org/54923003/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:168:
// Tests won't have a generator available.  We need to handle
On 2013/11/01 16:21:01, nyquist wrote:
> Native tests?

I wasn't planning on injecting a generator for non-native tests.  I know we had
discussed injecting the a generator for non-native tests, but that was before I
determined that we'd need the fallback for the native tests.  Since the fallback
handles all the non-native test cases too, I figured we didn't need to register
a generator for any tests.

Maybe we should talk about this.

https://codereview.chromium.org/54923003/diff/30001/sync/android/java/src/org...
File sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
(right):

https://codereview.chromium.org/54923003/diff/30001/sync/android/java/src/org...
sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java:94: //
Any intents sent to the InvalidationService should include the
On 2013/11/01 16:21:01, nyquist wrote:
> You have 100 characters to work with...

Done.

[nyquist, 2013-11-01 21:25:43]

mostly lg, but can you please write tests for the InvalidationController and
InvalidationService that tests the new functionality? Feel free to ping me if
you need hints on how to do that.

[rlarocque, 2013-11-02 00:31:00]

I had been running some tests, but apparently I wasn't running all the relevant
ones.  I had to fix a few broken tests before adding new ones.

The latest uploads include a new C++ test, some fixes for InvalidationService
tests, and some new InvalidationController tests.  Let me know if you'd like to
see anything else.

[nyquist, 2013-11-04 18:56:45]

You mentioned a new C++ test... I can't seem to find it though. Did you forget
to run 'git add'?

https://codereview.chromium.org/54923003/diff/8/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(right):

https://codereview.chromium.org/54923003/diff/8/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:159:
if (mUniqueId != null) {
Here you assume thread safety, but I can not find any thread safety measures in
this class. Add a lock, or assert the thread?

https://codereview.chromium.org/54923003/diff/8/chrome/android/javatests/src/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java
(right):

https://codereview.chromium.org/54923003/diff/8/chrome/android/javatests/src/...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:346:
String generator_id = generator.getUniqueId(null);
generatorId

https://codereview.chromium.org/54923003/diff/8/chrome/android/javatests/src/...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:403:
false);
I think you can force this, since you are running this from a test (ie. pass
true here).

[rlarocque, 2013-11-11 18:55:44]

I've 'git added' the C++ tests and addressed a few of your comments.  PTAL.

https://codereview.chromium.org/54923003/diff/8/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(right):

https://codereview.chromium.org/54923003/diff/8/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:159:
if (mUniqueId != null) {
On 2013/11/04 18:56:46, nyquist (GMT 11.4-11.18) wrote:
> Here you assume thread safety, but I can not find any thread safety measures
in
> this class. Add a lock, or assert the thread?

I was under the impression that this class is accessed only by the UI thread.  I
know that's true on the C++ side of things.  Is it not true of the Java side?

Anyway, I've added the "synchronized" keyword to this method.

https://codereview.chromium.org/54923003/diff/8/chrome/android/javatests/src/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java
(right):

https://codereview.chromium.org/54923003/diff/8/chrome/android/javatests/src/...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:346:
String generator_id = generator.getUniqueId(null);
On 2013/11/04 18:56:46, nyquist (GMT 11.4-11.18) wrote:
> generatorId

Done.

https://codereview.chromium.org/54923003/diff/8/chrome/android/javatests/src/...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:403:
false);
On 2013/11/04 18:56:46, nyquist (GMT 11.4-11.18) wrote:
> I think you can force this, since you are running this from a test (ie. pass
> true here).

Done.

[nyquist, 2013-11-11 19:20:39]

lgtm with one comment about not using the synchronized keyword in the method
signature.

https://codereview.chromium.org/54923003/diff/8/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(right):

https://codereview.chromium.org/54923003/diff/8/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:159:
if (mUniqueId != null) {
On 2013/11/11 18:55:44, rlarocque wrote:
> On 2013/11/04 18:56:46, nyquist (GMT 11.4-11.18) wrote:
> > Here you assume thread safety, but I can not find any thread safety
measures
> in
> > this class. Add a lock, or assert the thread?
> 
> I was under the impression that this class is accessed only by the UI thread. 
I
> know that's true on the C++ side of things.  Is it not true of the Java side?
> 
> Anyway, I've added the "synchronized" keyword to this method.

I believe currently it is only accessed on the UI thread, but I do not think
that is a guarantee.

https://codereview.chromium.org/54923003/diff/270001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(right):

https://codereview.chromium.org/54923003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:158:
synchronized public byte[] getInvalidatorClientId() {
Please do not use the synchronized keyword, but instead use a private final
field that you use for the lock. For example
private final Object mLock = new Object();

And then use it like:
public byte[] getInvalidatorClientId() {
    synchronized(mLock) {
        // your code
    }
}

[rlarocque, 2013-11-11 19:46:38]

https://codereview.chromium.org/54923003/diff/270001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java
(right):

https://codereview.chromium.org/54923003/diff/270001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationController.java:158:
synchronized public byte[] getInvalidatorClientId() {
On 2013/11/11 19:20:40, nyquist (GMT 11.4-11.18) wrote:
> Please do not use the synchronized keyword, but instead use a private final
> field that you use for the lock. For example
> private final Object mLock = new Object();
> 
> And then use it like:
> public byte[] getInvalidatorClientId() {
>     synchronized(mLock) {
>         // your code
>     }
> }

Done.

[commit-bot: I haz the power, 2013-11-11 19:50:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/54923003/340001

[commit-bot: I haz the power, 2013-11-11 20:34:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-11 21:00:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/54923003/610001

[commit-bot: I haz the power, 2013-11-12 00:14:05]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-12 00:32:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/54923003/610001

[commit-bot: I haz the power, 2013-11-12 04:49:34]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-12 14:25:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/54923003/610001

[commit-bot: I haz the power, 2013-11-12 15:36:32]

Change committed as 234524