Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557273004:
android: Randomize initial child service bind order (Closed)
Descriptionandroid: Randomize initial child service bind order
Android restarts crashed services with the previous intent.
If browser crashes and is restarted at the same time, then
it's possible to bind to the restarted service with stale
intent and bundle data. See crbug.com/664341#c83 for an
example. Randomizing the start up order is a pure
workaround to make this case less likely.
BUG=664341
Committed: https://crrev.com/bb725fed9bd8c672178c8a815f4b1a2500eef444
Cr-Commit-Position: refs/heads/master@{#437367}
Patch Set 1 #Patch Set 2 : fix test #
Total comments: 4
Patch Set 3 : assertNotnull #
Messages
Total messages: 38 (20 generated)
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
boliu@chromium.org changed reviewers: + agrieve@chromium.org, jcivelli@chromium.org, mariakhomenko@chromium.org
ptal small and safe?
lgtm I don't see any issues with this.
The CQ bit was unchecked by boliu@chromium.org
boliu@chromium.org changed reviewers: + hanxi@chromium.org
+hanxi, looks like this is breaking testServiceNumberAllocation is order important for webapks?
On 2016/12/08 19:21:28, boliu wrote: > +hanxi, looks like this is breaking testServiceNumberAllocation > > is order important for webapks? actually, not really, it's just verifying the two connections are coming from a different connection pools, can probably write it some other way..
On 2016/12/08 19:21:28, boliu wrote: > +hanxi, looks like this is breaking testServiceNumberAllocation > > is order important for webapks? I don't think the order is important, only how many connection is allocated is important in that test.
On 2016/12/08 19:33:35, Xi Han wrote: > On 2016/12/08 19:21:28, boliu wrote: > > +hanxi, looks like this is breaking testServiceNumberAllocation > > > > is order important for webapks? > > I don't think the order is important, only how many connection is allocated is > important in that test. Test is already verifying number of connections from different pools, so that should be enough. So I just removed the service number checks. anyone wanna stamp the test change?
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2557273004/#ps20001 (title: "fix test")
The CQ bit was unchecked by boliu@chromium.org
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
oops, accidentally clicked cq rather than dry run
Tests lgtm.
https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:245: @SuppressFBWarnings("DLS_DEAD_LOCAL_STORE") rather than suppress warnings, why do you need to store the results of allocateConnection? Just call it without saving the result?
https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:245: @SuppressFBWarnings("DLS_DEAD_LOCAL_STORE") On 2016/12/08 19:51:37, Maria wrote: > rather than suppress warnings, why do you need to store the results of > allocateConnection? Just call it without saving the result? That assumes connection impl doesn't have like some finalizer that cleans up things when nothing is referencing them, stuff like that. I think test holding onto the connection for the duration of the test actually is the right thing to do.
https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:245: @SuppressFBWarnings("DLS_DEAD_LOCAL_STORE") On 2016/12/08 19:53:20, boliu wrote: > On 2016/12/08 19:51:37, Maria wrote: > > rather than suppress warnings, why do you need to store the results of > > allocateConnection? Just call it without saving the result? > > That assumes connection impl doesn't have like some finalizer that cleans up > things when nothing is referencing them, stuff like that. I think test holding > onto the connection for the duration of the test actually is the right thing to > do. Okay, that's fair. How about asserting they are not null then?
https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:245: @SuppressFBWarnings("DLS_DEAD_LOCAL_STORE") On 2016/12/08 19:57:59, Maria wrote: > On 2016/12/08 19:53:20, boliu wrote: > > On 2016/12/08 19:51:37, Maria wrote: > > > rather than suppress warnings, why do you need to store the results of > > > allocateConnection? Just call it without saving the result? > > > > That assumes connection impl doesn't have like some finalizer that cleans up > > things when nothing is referencing them, stuff like that. I think test holding > > onto the connection for the duration of the test actually is the right thing > to > > do. > > Okay, that's fair. How about asserting they are not null then? Ahh, good idea. Done.
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== android: Randomize initial child service bind order Android restarts crashed services with the previous intent. If browser crashes and is restarted at the same time, then it's possible to bind to the restarted service with stale intent and bundle data. See crbug.com/664341#c84 for an example. Randomizing the start up order is a pure workaround to make this case less likely. BUG=664341 ========== to ========== android: Randomize initial child service bind order Android restarts crashed services with the previous intent. If browser crashes and is restarted at the same time, then it's possible to bind to the restarted service with stale intent and bundle data. See crbug.com/664341#c83 for an example. Randomizing the start up order is a pure workaround to make this case less likely. BUG=664341 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2016/12/08 20:01:32, boliu wrote: > https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... > File > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java > (right): > > https://codereview.chromium.org/2557273004/diff/20001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:245: > @SuppressFBWarnings("DLS_DEAD_LOCAL_STORE") > On 2016/12/08 19:57:59, Maria wrote: > > On 2016/12/08 19:53:20, boliu wrote: > > > On 2016/12/08 19:51:37, Maria wrote: > > > > rather than suppress warnings, why do you need to store the results of > > > > allocateConnection? Just call it without saving the result? > > > > > > That assumes connection impl doesn't have like some finalizer that cleans up > > > things when nothing is referencing them, stuff like that. I think test > holding > > > onto the connection for the duration of the test actually is the right thing > > to > > > do. > > > > Okay, that's fair. How about asserting they are not null then? > > Ahh, good idea. Done. Maria: ok to cq?
lgtm
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org Link to the patchset: https://codereview.chromium.org/2557273004/#ps40001 (title: "assertNotnull")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481234639346300,
"parent_rev": "39e45595851a8c1e0fd80c6c28204807c248bb9f", "commit_rev":
"19654187930adadcf518a654f18192a935d5b1a8"}
Message was sent while issue was closed.
Description was changed from ========== android: Randomize initial child service bind order Android restarts crashed services with the previous intent. If browser crashes and is restarted at the same time, then it's possible to bind to the restarted service with stale intent and bundle data. See crbug.com/664341#c83 for an example. Randomizing the start up order is a pure workaround to make this case less likely. BUG=664341 ========== to ========== android: Randomize initial child service bind order Android restarts crashed services with the previous intent. If browser crashes and is restarted at the same time, then it's possible to bind to the restarted service with stale intent and bundle data. See crbug.com/664341#c83 for an example. Randomizing the start up order is a pure workaround to make this case less likely. BUG=664341 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== android: Randomize initial child service bind order Android restarts crashed services with the previous intent. If browser crashes and is restarted at the same time, then it's possible to bind to the restarted service with stale intent and bundle data. See crbug.com/664341#c83 for an example. Randomizing the start up order is a pure workaround to make this case less likely. BUG=664341 ========== to ========== android: Randomize initial child service bind order Android restarts crashed services with the previous intent. If browser crashes and is restarted at the same time, then it's possible to bind to the restarted service with stale intent and bundle data. See crbug.com/664341#c83 for an example. Randomizing the start up order is a pure workaround to make this case less likely. BUG=664341 Committed: https://crrev.com/bb725fed9bd8c672178c8a815f4b1a2500eef444 Cr-Commit-Position: refs/heads/master@{#437367} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bb725fed9bd8c672178c8a815f4b1a2500eef444 Cr-Commit-Position: refs/heads/master@{#437367}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2571723002/ by boliu@chromium.org. The reason for reverting is: Android keeps the intent for a long time, so randomize doesn't help at all. More details: crbug.com/664341#c91. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
