Make InstanceIDBridge fully async to fix strict mode violations

Make InstanceIDBridge fully async to fix strict mode violations

InstanceID.java's getInstance/getId/getCreationTime methods are not
documented as needing to be called from a background thread (unlike
getToken and deleteToken which are documented as "Do not call this
function on the main thread.", or deleteInstanceID which is not
documented but at least throws an IOException if you get this wrong).

However it turns out that getInstance/getId/getCreationTime can all
cause strict mode violations if called on the main thread, because
they sometimes read from SharedPreferences (and hence from disk).

This patch fixes these violations, by making InstanceIDBridge fully
asynchronous. Its InstanceIDWithSubtype is now initialized lazily on a
background thread.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. this patch
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
Committed: https://crrev.com/6ab9853acfbfae3e598004776c1d120805956038
Cr-Commit-Position: refs/heads/master@{#390139}

[johnme, 2016-04-18 14:22:02]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2016-04-18 15:49:44]

lgtm

https://codereview.chromium.org/1899753002/diff/1/components/gcm_driver/insta...
File
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1899753002/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:64:
// thread, by reading from SharedPreferences.
I don't think this is very useful to document here, dito re: 77. Maybe send an
e-mail to tingmui@ to get the docs fixed?

[johnme, 2016-04-19 12:33:00]

https://codereview.chromium.org/1899753002/diff/1/components/gcm_driver/insta...
File
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1899753002/diff/1/components/gcm_driver/insta...
components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:64:
// thread, by reading from SharedPreferences.
On 2016/04/18 15:49:44, Peter Beverloo wrote:
> I don't think this is very useful to document here, dito re: 77. Maybe send an
> e-mail to tingmui@ to get the docs fixed?

It's helpful to understand why the fake throws an exception here (and more
importantly, what the code is doing wrong if it triggers this exception). I've
filed b/28259183 to get the docs fixed.

[johnme, 2016-04-26 14:52:20]

Hey Peter, PTAL at last patch that fixes a use after free.

[Peter Beverloo, 2016-04-26 15:12:23]

https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.cc (right):

https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:216:
DeleteTokenCallback* callback = delete_id_callbacks_.Lookup(request_id);
Where does this happen? The three calls to InstanceID::DeleteID() all look fine
to me. 

The InstanceID instances are owned by the InstanceIDDriver, so this would only
occur when someone deletes their ID and then, in the callback handler, calls
InstanceIDDriver::RemoveInstanceID. The only candidate is
ExtensionGCMAppHandler, which does this through a PostTask().

In general, this problem is not unique to DidDeleteID, but would apply to any of
the asynchronous methods in this implementation.

Instead, could we add a class-level comment that the callbacks should *not*
synchronously remove the instances and leave it at that?

[johnme, 2016-04-26 17:18:28]

https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.cc (right):

https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:216:
DeleteTokenCallback* callback = delete_id_callbacks_.Lookup(request_id);
On 2016/04/26 15:12:23, Peter Beverloo wrote:
> Where does this happen? The three calls to InstanceID::DeleteID() all look
fine
> to me. 
> 
> The InstanceID instances are owned by the InstanceIDDriver, so this would only
> occur when someone deletes their ID and then, in the callback handler, calls
> InstanceIDDriver::RemoveInstanceID. The only candidate is
> ExtensionGCMAppHandler, which does this through a PostTask().
> 
> In general, this problem is not unique to DidDeleteID, but would apply to any
of
> the asynchronous methods in this implementation.
> 
> Instead, could we add a class-level comment that the callbacks should *not*
> synchronously remove the instances and leave it at that?

The pattern set by the only existing user of IIDs (ExtensionGCMAppHandler), is
that you call InstanceIDDriver::RemoveInstanceID after InstanceID::DeleteID
completes (this is a bit of a silly pattern, but refactoring it is a task for
another day). Hence I do the same in
https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...,
though I happened to omit the PostTask, and that seems an easy mistake to make
(even with a class comment).

So I'd rather be defensive here. An alternative would be to run the DeleteID
callback using PostTask, if you think that's better.

I'm less concerned about the other methods, since there's no strong reason to
call RemoveInstanceID from their callbacks, though I'll add a class comment. I
also toned down the comments here from usually/likely to maybe.

[Peter Beverloo, 2016-04-26 17:27:28]

peter@chromium.org changed reviewers:
+ jianli@chromium.org

[Peter Beverloo, 2016-04-26 17:27:28]

+jianli, would you mind being the tie-breaker?

(relevant context in the following comment and instance_id_android.cc:216)

On 2016/04/26 17:18:28, johnme wrote:
>
https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
> File components/gcm_driver/instance_id/instance_id_android.cc (right):
> 
>
https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
> components/gcm_driver/instance_id/instance_id_android.cc:216:
> DeleteTokenCallback* callback = delete_id_callbacks_.Lookup(request_id);
> On 2016/04/26 15:12:23, Peter Beverloo wrote:
> > Where does this happen? The three calls to InstanceID::DeleteID() all look
> fine
> > to me. 
> > 
> > The InstanceID instances are owned by the InstanceIDDriver, so this would
only
> > occur when someone deletes their ID and then, in the callback handler, calls
> > InstanceIDDriver::RemoveInstanceID. The only candidate is
> > ExtensionGCMAppHandler, which does this through a PostTask().
> > 
> > In general, this problem is not unique to DidDeleteID, but would apply to
any
> of
> > the asynchronous methods in this implementation.
> > 
> > Instead, could we add a class-level comment that the callbacks should *not*
> > synchronously remove the instances and leave it at that?
> 
> The pattern set by the only existing user of IIDs (ExtensionGCMAppHandler), is
> that you call InstanceIDDriver::RemoveInstanceID after InstanceID::DeleteID
> completes (this is a bit of a silly pattern, but refactoring it is a task for
> another day). Hence I do the same in
>
https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...,
> though I happened to omit the PostTask, and that seems an easy mistake to make
> (even with a class comment).
> 
> So I'd rather be defensive here. An alternative would be to run the DeleteID
> callback using PostTask, if you think that's better.
> 
> I'm less concerned about the other methods, since there's no strong reason to
> call RemoveInstanceID from their callbacks, though I'll add a class comment. I
> also toned down the comments here from usually/likely to maybe.

I understand your concern and arguments, I just don't agree that special-casing
the DeleteID + RemoveInstanceID is the right solution here when the other
methods have the exact same problem.

Instead, I still think we should either (a) document the
don't-synchronously-remove contract, or (b) solve it for all methods, with a
mild preference towards the former.

[johnme, 2016-04-26 17:31:32]

On 2016/04/26 17:27:28, Peter Beverloo wrote:
> +jianli, would you mind being the tie-breaker?
> 
> (relevant context in the following comment and instance_id_android.cc:216)
> 
> On 2016/04/26 17:18:28, johnme wrote:
> >
>
https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
> > File components/gcm_driver/instance_id/instance_id_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/1899753002/diff/40001/components/gcm_driver/i...
> > components/gcm_driver/instance_id/instance_id_android.cc:216:
> > DeleteTokenCallback* callback = delete_id_callbacks_.Lookup(request_id);
> > On 2016/04/26 15:12:23, Peter Beverloo wrote:
> > > Where does this happen? The three calls to InstanceID::DeleteID() all look
> > fine
> > > to me. 
> > > 
> > > The InstanceID instances are owned by the InstanceIDDriver, so this would
> only
> > > occur when someone deletes their ID and then, in the callback handler,
calls
> > > InstanceIDDriver::RemoveInstanceID. The only candidate is
> > > ExtensionGCMAppHandler, which does this through a PostTask().
> > > 
> > > In general, this problem is not unique to DidDeleteID, but would apply to
> any
> > of
> > > the asynchronous methods in this implementation.
> > > 
> > > Instead, could we add a class-level comment that the callbacks should
*not*
> > > synchronously remove the instances and leave it at that?
> > 
> > The pattern set by the only existing user of IIDs (ExtensionGCMAppHandler),
is
> > that you call InstanceIDDriver::RemoveInstanceID after InstanceID::DeleteID
> > completes (this is a bit of a silly pattern, but refactoring it is a task
for
> > another day). Hence I do the same in
> >
>
https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...,
> > though I happened to omit the PostTask, and that seems an easy mistake to
make
> > (even with a class comment).
> > 
> > So I'd rather be defensive here. An alternative would be to run the DeleteID
> > callback using PostTask, if you think that's better.
> > 
> > I'm less concerned about the other methods, since there's no strong reason
to
> > call RemoveInstanceID from their callbacks, though I'll add a class comment.
I
> > also toned down the comments here from usually/likely to maybe.
> 
> I understand your concern and arguments, I just don't agree that
special-casing
> the DeleteID + RemoveInstanceID is the right solution here when the other
> methods have the exact same problem.
> 
> Instead, I still think we should either (a) document the
> don't-synchronously-remove contract, or (b) solve it for all methods, with a
> mild preference towards the former.

FWIW, the latest patch does document it (as well as special-casing DeleteID).
For (b), I guess we could just PostTask all the callbacks.

[jianli, 2016-04-26 20:50:56]

https://codereview.chromium.org/1899753002/diff/60001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.h (right):

https://codereview.chromium.org/1899753002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.h:25: // mustn't
synchronously delete this (using InstanceIDDriver::RemoveInstanceID).
I don't think we want to allow synchronous deleting InstanceID instance from the
callback. ExtensionGCMAppHandler::OnDeleteIDCompleted already has the logic for
this. For all other usages, we should also try to do this.

[johnme, 2016-04-27 09:41:56]

Addressed review comment - thanks!

https://codereview.chromium.org/1899753002/diff/60001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.h (right):

https://codereview.chromium.org/1899753002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.h:25: // mustn't
synchronously delete this (using InstanceIDDriver::RemoveInstanceID).
On 2016/04/26 20:50:55, jianli wrote:
> I don't think we want to allow synchronous deleting InstanceID instance from
the
> callback. ExtensionGCMAppHandler::OnDeleteIDCompleted already has the logic
for
> this. For all other usages, we should also try to do this.

Ok, done. DeleteID no longer supports synchronous delete from its callback.
Instead I just added a comment to instance_id.h documenting that callbacks
mustn't delete the IID.

[Peter Beverloo, 2016-04-27 10:56:37]

lgtm, thank you :)

https://codereview.chromium.org/1899753002/diff/80001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1899753002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:44: // Asynchronous callbacks.
Must not synchronously delete this (using
nit: reads slightly easier if you'd use |this| (zeh pipes)

[johnme, 2016-04-27 17:59:51]

Description was changed from

==========
Make InstanceIDBridge fully async to fix strict mode violations

InstanceID.java's getInstance/getId/getCreationTime methods are not
documented as needing to be called from a background thread (unlike
getToken and deleteToken which are documented as "Do not call this
function on the main thread.", or deleteInstanceID which is not
documented but at least throws an IOException if you get this wrong).

However it turns out that getInstance/getId/getCreationTime can all
cause strict mode violations if called on the main thread, because
they sometimes read from SharedPreferences (and hence from disk).

This patch fixes these violations, by making InstanceIDBridge fully
asynchronous. Its InstanceIDWithSubtype is now initialized lazily on a
background thread.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. this patch
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Make InstanceIDBridge fully async to fix strict mode violations

InstanceID.java's getInstance/getId/getCreationTime methods are not
documented as needing to be called from a background thread (unlike
getToken and deleteToken which are documented as "Do not call this
function on the main thread.", or deleteInstanceID which is not
documented but at least throws an IOException if you get this wrong).

However it turns out that getInstance/getId/getCreationTime can all
cause strict mode violations if called on the main thread, because
they sometimes read from SharedPreferences (and hence from disk).

This patch fixes these violations, by making InstanceIDBridge fully
asynchronous. Its InstanceIDWithSubtype is now initialized lazily on a
background thread.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. this patch
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[johnme, 2016-04-27 18:03:57]

https://codereview.chromium.org/1899753002/diff/80001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1899753002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:44: // Asynchronous callbacks.
Must not synchronously delete this (using
On 2016/04/27 10:56:37, Peter Beverloo wrote:
> nit: reads slightly easier if you'd use |this| (zeh pipes)

Done.

[johnme, 2016-04-27 18:04:03]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-04-27 18:04:03]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1899753002/#ps100001
(title: "|this|")

[commit-bot: I haz the power, 2016-04-27 18:04:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1899753002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1899753002/100001

[commit-bot: I haz the power, 2016-04-27 18:57:22]

Description was changed from

==========
Make InstanceIDBridge fully async to fix strict mode violations

InstanceID.java's getInstance/getId/getCreationTime methods are not
documented as needing to be called from a background thread (unlike
getToken and deleteToken which are documented as "Do not call this
function on the main thread.", or deleteInstanceID which is not
documented but at least throws an IOException if you get this wrong).

However it turns out that getInstance/getId/getCreationTime can all
cause strict mode violations if called on the main thread, because
they sometimes read from SharedPreferences (and hence from disk).

This patch fixes these violations, by making InstanceIDBridge fully
asynchronous. Its InstanceIDWithSubtype is now initialized lazily on a
background thread.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. this patch
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Make InstanceIDBridge fully async to fix strict mode violations

InstanceID.java's getInstance/getId/getCreationTime methods are not
documented as needing to be called from a background thread (unlike
getToken and deleteToken which are documented as "Do not call this
function on the main thread.", or deleteInstanceID which is not
documented but at least throws an IOException if you get this wrong).

However it turns out that getInstance/getId/getCreationTime can all
cause strict mode violations if called on the main thread, because
they sometimes read from SharedPreferences (and hence from disk).

This patch fixes these violations, by making InstanceIDBridge fully
asynchronous. Its InstanceIDWithSubtype is now initialized lazily on a
background thread.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. this patch
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[commit-bot: I haz the power, 2016-04-27 18:57:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:11:37]

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

[commit-bot: I haz the power, 2016-04-30 17:51:06]

Description was changed from

==========
Make InstanceIDBridge fully async to fix strict mode violations

InstanceID.java's getInstance/getId/getCreationTime methods are not
documented as needing to be called from a background thread (unlike
getToken and deleteToken which are documented as "Do not call this
function on the main thread.", or deleteInstanceID which is not
documented but at least throws an IOException if you get this wrong).

However it turns out that getInstance/getId/getCreationTime can all
cause strict mode violations if called on the main thread, because
they sometimes read from SharedPreferences (and hence from disk).

This patch fixes these violations, by making InstanceIDBridge fully
asynchronous. Its InstanceIDWithSubtype is now initialized lazily on a
background thread.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. this patch
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Make InstanceIDBridge fully async to fix strict mode violations

InstanceID.java's getInstance/getId/getCreationTime methods are not
documented as needing to be called from a background thread (unlike
getToken and deleteToken which are documented as "Do not call this
function on the main thread.", or deleteInstanceID which is not
documented but at least throws an IOException if you get this wrong).

However it turns out that getInstance/getId/getCreationTime can all
cause strict mode violations if called on the main thread, because
they sometimes read from SharedPreferences (and hence from disk).

This patch fixes these violations, by making InstanceIDBridge fully
asynchronous. Its InstanceIDWithSubtype is now initialized lazily on a
background thread.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. this patch
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1923953002 adds crypto integration
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461

Committed: https://crrev.com/6ab9853acfbfae3e598004776c1d120805956038
Cr-Commit-Position: refs/heads/master@{#390139}
==========