|
|
Created:
4 years, 8 months ago by johnme Modified:
4 years, 7 months ago CC:
chromium-reviews, Peter Beverloo, johnme+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@iid3test Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake 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}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #Patch Set 3 : Fix DeleteID use after free #
Total comments: 2
Patch Set 4 : Rebase & tweak lifetime comments #
Total comments: 2
Patch Set 5 : Disallow synchronous delete from callbacks #
Total comments: 2
Patch Set 6 : |this| #
Dependent Patchsets: Messages
Total messages: 22 (7 generated)
[email protected] changed reviewers: + [email protected]
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?
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.
Hey Peter, PTAL at last patch that fixes a use after free.
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?
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.
[email protected] changed reviewers: + [email protected]
+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.
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.
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.
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.
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)
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 ==========
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.
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected] Link to the patchset: https://codereview.chromium.org/1899753002/#ps100001 (title: "|this|")
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6ab9853acfbfae3e598004776c1d120805956038 Cr-Commit-Position: refs/heads/master@{#390139}
Message was sent while issue was closed.
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} ========== |