Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2470913005:
Fix the blimp settings crash. (Closed)
|
Created:
4 years, 1 month ago by xingliu Modified:
4 years, 1 month ago CC:
amineer, chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the blimp settings crash.
The assignment fetcher is not initialized when the settings jni bridge
needs it, which causes the crash.
To reproduce the bug, we need to clear adb_chrome_public_command_line.
Run:
build/android/adb_chrome_public_command_line ""
Intialization detail:
Since assignment fetcher and identity source both need identity
provider, which is a service created from BlimpClientContextDelegate,
they can't be created in the ctor of BlimpClientContextImpl.
With another CL: https://codereview.chromium.org/2471733002/
We should be able to run local server with client_engine_integration
script, but currently there might be other bugs that local client still
can't receive messages from local engine.
BUG=661322
Committed: https://crrev.com/6a658f84fc845447726b451b2e26dbd043cea5c6
Cr-Commit-Position: refs/heads/master@{#429618}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Minor polish. #Patch Set 3 : Put init code into SetDelegate. #
Total comments: 9
Patch Set 4 : Polish the SetDelegate call. #
Messages
Total messages: 45 (22 generated)
Description was changed from ========== Fix the blimp settings crash. The assignment fetcher is not initialized when the settings jni bridge needs it, which causes the crash. To reproduce the bug, we need to clear adb_chrome_public_command_line. Since assignment fecther and identity source both need identity provider, which is a service created from BlimpClientContextDelegate, they can't be created in the ctor of BlimpClientContextImpl. With another CL: https://codereview.chromium.org/2471733002/ We should be able to run local server with client_engine_integration script, but currently there might be other bugs that local client still can't receive messages from local engine. BUG=661322 ========== to ========== Fix the blimp settings crash. The assignment fetcher is not initialized when the settings jni bridge needs it, which causes the crash. To reproduce the bug, we need to clear adb_chrome_public_command_line. Run: build/android/adb_chrome_public_command_line "" Since assignment fecther and identity source both need identity provider, which is a service created from BlimpClientContextDelegate, they can't be created in the ctor of BlimpClientContextImpl. With another CL: https://codereview.chromium.org/2471733002/ We should be able to run local server with client_engine_integration script, but currently there might be other bugs that local client still can't receive messages from local engine. BUG=661322 ==========
Description was changed from ========== Fix the blimp settings crash. The assignment fetcher is not initialized when the settings jni bridge needs it, which causes the crash. To reproduce the bug, we need to clear adb_chrome_public_command_line. Run: build/android/adb_chrome_public_command_line "" Since assignment fecther and identity source both need identity provider, which is a service created from BlimpClientContextDelegate, they can't be created in the ctor of BlimpClientContextImpl. With another CL: https://codereview.chromium.org/2471733002/ We should be able to run local server with client_engine_integration script, but currently there might be other bugs that local client still can't receive messages from local engine. BUG=661322 ========== to ========== Fix the blimp settings crash. The assignment fetcher is not initialized when the settings jni bridge needs it, which causes the crash. To reproduce the bug, we need to clear adb_chrome_public_command_line. Run: build/android/adb_chrome_public_command_line "" Intialization detail: Since assignment fetcher and identity source both need identity provider, which is a service created from BlimpClientContextDelegate, they can't be created in the ctor of BlimpClientContextImpl. With another CL: https://codereview.chromium.org/2471733002/ We should be able to run local server with client_engine_integration script, but currently there might be other bugs that local client still can't receive messages from local engine. BUG=661322 ==========
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, khushalsagar@chromium.org, nyquist@chromium.org
Fix the setting crash, caused by assignment_fetcher.
The CQ bit was checked by xingliu@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...
https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... blimp/client/core/context/blimp_client_context_impl.cc:260: assignment_fetcher_ = base::MakeUnique<AssignmentFetcher>( Can we create this in SetDelegate instead then? Rather than having the if check everywhere?
lgtm https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... File blimp/client/core/context/blimp_client_context_impl.h (right): https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... blimp/client/core/context/blimp_client_context_impl.h:95: void CreateAssignmentFetcher(); CreateAssignmentFetcherIfNeeded?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm if you want to land the fix first quickly. My suggestion would be to set up the lifetime clearly if possible. The delagate_ is given to the context later, but is provided only once and then will outlive the context. At least that's how I'm understanding it. So do all the initializations that depend on the delegate in SetDelegate and DCHECK there to make sure it is called only once, and may be add a HasDelegate() method that you DCHECK in every other call, it becomes obvious that they should never be made till the context is initialized with the delegate. Makes debugging things easier too. :)
The CQ bit was checked by xingliu@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...
On 2016/11/02 22:52:43, Khushal wrote: > lgtm if you want to land the fix first quickly. > > My suggestion would be to set up the lifetime clearly if possible. The delagate_ > is given to the context later, but is provided only once and then will outlive > the context. At least that's how I'm understanding it. > > So do all the initializations that depend on the delegate in SetDelegate and > DCHECK there to make sure it is called only once, and may be add a HasDelegate() > method that you DCHECK in every other call, it becomes obvious that they should > never be made till the context is initialized with the delegate. Makes debugging > things easier too. :) Makes sense, but I'm wondering if there is any assumption with the delegate here.
Minor polish, currently didn't touch the SetDelegate, I think we might reiterate the life cycle of delegate and context. https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... blimp/client/core/context/blimp_client_context_impl.cc:260: assignment_fetcher_ = base::MakeUnique<AssignmentFetcher>( On 2016/11/02 21:29:56, Khushal wrote: > Can we create this in SetDelegate instead then? Rather than having the if check > everywhere? @nyquist, I think do it in SetDelegate makes sense, but is there any assumption in SetDelegate that we only can called it for once, or BlimpClientContext can run without a delegate. Since Multi window android can potentially have two ChromeTabbedActivity(ChromeTabbedActivity2 for the second window), so SetDelegate can get called twice. https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... File blimp/client/core/context/blimp_client_context_impl.h (right): https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... blimp/client/core/context/blimp_client_context_impl.h:95: void CreateAssignmentFetcher(); On 2016/11/02 21:30:08, nyquist wrote: > CreateAssignmentFetcherIfNeeded? Done.
https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... blimp/client/core/context/blimp_client_context_impl.cc:254: if (assignment_fetcher_) nit: Why do the if check at the call sites if you're doing it here anyway? https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... blimp/client/core/context/blimp_client_context_impl.cc:260: assignment_fetcher_ = base::MakeUnique<AssignmentFetcher>( On 2016/11/02 23:26:52, xingliu wrote: > On 2016/11/02 21:29:56, Khushal wrote: > > Can we create this in SetDelegate instead then? Rather than having the if > check > > everywhere? > > @nyquist, I think do it in SetDelegate makes sense, but is there any assumption > in SetDelegate that we only can called it for once, or BlimpClientContext can > run without a delegate. Definitely can't do without a |delegate_|, you need it for the |assignment_fetcher_| for instance. :) > > Since Multi window android can potentially have two > ChromeTabbedActivity(ChromeTabbedActivity2 for the second window), so > SetDelegate can get called twice. Hmmm, well, there should be only one delegate for a context at any instant for sure. Reading from here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... And when the delegate is dying it will reset its pointer to you. Shouldn't we destroy everything we initialized using the delegate at this point? I would expect that even if after this the context gets re-used by another delegate. So you have a clear initialization sequence where you create everything tied to a delegate when you get it, and destroy the same things when its going away. And for the calls, like Connect, which should be made only after the context has been provided with a delegate, a HasDelegate check would help anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Minor polish, put the initialization of assignment fetcher into SetDelegate. I'll put it into CQ, so we can have build tomorrow morning. https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/1/blimp/client/core/context/b... blimp/client/core/context/blimp_client_context_impl.cc:260: assignment_fetcher_ = base::MakeUnique<AssignmentFetcher>( On 2016/11/02 23:45:45, Khushal wrote: > On 2016/11/02 23:26:52, xingliu wrote: > > On 2016/11/02 21:29:56, Khushal wrote: > > > Can we create this in SetDelegate instead then? Rather than having the if > > check > > > everywhere? > > > > @nyquist, I think do it in SetDelegate makes sense, but is there any > assumption > > in SetDelegate that we only can called it for once, or BlimpClientContext can > > run without a delegate. > > Definitely can't do without a |delegate_|, you need it for the > |assignment_fetcher_| for instance. :) > > > > > Since Multi window android can potentially have two > > ChromeTabbedActivity(ChromeTabbedActivity2 for the second window), so > > SetDelegate can get called twice. > > Hmmm, well, there should be only one delegate for a context at any instant for > sure. Reading from here: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > And when the delegate is dying it will reset its pointer to you. Shouldn't we > destroy everything we initialized using the delegate at this point? I would > expect that even if after this the context gets re-used by another delegate. So > you have a clear initialization sequence where you create everything tied to a > delegate when you get it, and destroy the same things when its going away. > > And for the calls, like Connect, which should be made only after the context has > been provided with a delegate, a HasDelegate check would help anyway. Yeah, agree. Put it into SetDelegate. Since the delegate now is always needed.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, khushalsagar@chromium.org Link to the patchset: https://codereview.chromium.org/2470913005/#ps40001 (title: "Put init code into SetDelegate.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... blimp/client/core/context/blimp_client_context_impl.cc:150: void BlimpClientContextImpl::SetDelegate(BlimpClientContextDelegate* delegate) { This is being called with a null delegate from its dtor. See https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim... So what you want to is: DCHECK(!delegate_ || !delegate), basically one of them should be null. Then, if (delegate_) { assignment_fetcher_ = nullptr; } delegate_ = delegate; if (delegate_) { assignment_fetcher_ = // Create the fetcher. }
The CQ bit was unchecked by khushalsagar@chromium.org
https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... blimp/client/public/blimp_client_context.h:69: // We must call SetDelegate right after the constructor. nit: The context must be initialized with a |delegate| before it can be used.
https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... blimp/client/core/context/blimp_client_context_impl.cc:150: void BlimpClientContextImpl::SetDelegate(BlimpClientContextDelegate* delegate) { On 2016/11/03 00:13:35, Khushal wrote: > This is being called with a null delegate from its dtor. See > https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim... > > So what you want to is: > DCHECK(!delegate_ || !delegate), basically one of them should be null. Then, > > if (delegate_) { > assignment_fetcher_ = nullptr; > } > delegate_ = delegate; > if (delegate_) { > assignment_fetcher_ = // Create the fetcher. > } This makes sense to me. https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... blimp/client/public/blimp_client_context.h:69: // We must call SetDelegate right after the constructor. On 2016/11/03 00:17:24, Khushal wrote: > nit: The context must be initialized with a |delegate| before it can be used. Should this also be mentioned in the constructor comment then?
https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... blimp/client/public/blimp_client_context.h:69: // We must call SetDelegate right after the constructor. On 2016/11/03 00:21:40, nyquist wrote: > On 2016/11/03 00:17:24, Khushal wrote: > > nit: The context must be initialized with a |delegate| before it can be used. > > Should this also be mentioned in the constructor comment then? That makes sense. In fact mentioning it there should be enough.
https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... blimp/client/core/context/blimp_client_context_impl.cc:150: void BlimpClientContextImpl::SetDelegate(BlimpClientContextDelegate* delegate) { On 2016/11/03 00:13:35, Khushal wrote: > This is being called with a null delegate from its dtor. See > https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim... > > So what you want to is: > DCHECK(!delegate_ || !delegate), basically one of them should be null. Then, > > if (delegate_) { > assignment_fetcher_ = nullptr; > } > delegate_ = delegate; > if (delegate_) { > assignment_fetcher_ = // Create the fetcher. > } Sorry I didn't notice in dtor it sets to nullptr. But assignment_fetcher_can probably out live the delegate? https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/public/bli... blimp/client/public/blimp_client_context.h:69: // We must call SetDelegate right after the constructor. On 2016/11/03 00:21:40, nyquist wrote: > On 2016/11/03 00:17:24, Khushal wrote: > > nit: The context must be initialized with a |delegate| before it can be used. > > Should this also be mentioned in the constructor comment then? Done.
https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... blimp/client/core/context/blimp_client_context_impl.cc:150: void BlimpClientContextImpl::SetDelegate(BlimpClientContextDelegate* delegate) { On 2016/11/03 00:33:31, xingliu wrote: > On 2016/11/03 00:13:35, Khushal wrote: > > This is being called with a null delegate from its dtor. See > > > https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim... > > > > So what you want to is: > > DCHECK(!delegate_ || !delegate), basically one of them should be null. Then, > > > > if (delegate_) { > > assignment_fetcher_ = nullptr; > > } > > delegate_ = delegate; > > if (delegate_) { > > assignment_fetcher_ = // Create the fetcher. > > } > > Sorry I didn't notice in dtor it sets to nullptr. > But assignment_fetcher_can probably out live the delegate? I just looked at that class, and yes, it does look like the fetcher doesn't have anything to do with the delegate. But in that case, I'm super confused why we have a delegate callback for creating the IdentitySource. Why don't we pass it in the context's ctor, like the other things tied to the Profile when creating it. For instance, the PrefService is passed at that time here: https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/blimp_clien... So why not create the IdentityProvider there as well, base::WrapUnique<IdentityProvider>(new ProfileIdentityProvider( SigninManagerFactory::GetForProfile(profile_), ProfileOAuth2TokenServiceFactory::GetForProfile(profile_), base::Closure())) You can get profile the same way, Profile::FromBrowserContext(context). Am I missing something here?
Thanks for the feedback, it's better to create IdentityProvider early on, filed a bug for that. But it won't be done in this CL, this CL mainly targets to fix the crash bug which blocks other people. https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... File blimp/client/core/context/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... blimp/client/core/context/blimp_client_context_impl.cc:150: void BlimpClientContextImpl::SetDelegate(BlimpClientContextDelegate* delegate) { On 2016/11/03 00:50:48, Khushal wrote: > On 2016/11/03 00:33:31, xingliu wrote: > > On 2016/11/03 00:13:35, Khushal wrote: > > > This is being called with a null delegate from its dtor. See > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim... > > > > > > So what you want to is: > > > DCHECK(!delegate_ || !delegate), basically one of them should be null. Then, > > > > > > if (delegate_) { > > > assignment_fetcher_ = nullptr; > > > } > > > delegate_ = delegate; > > > if (delegate_) { > > > assignment_fetcher_ = // Create the fetcher. > > > } > > > > Sorry I didn't notice in dtor it sets to nullptr. > > But assignment_fetcher_can probably out live the delegate? > > I just looked at that class, and yes, it does look like the fetcher doesn't have > anything to do with the delegate. But in that case, I'm super confused why we > have a delegate callback for creating the IdentitySource. Why don't we pass it > in the context's ctor, like the other things tied to the Profile when creating > it. > For instance, the PrefService is passed at that time here: > https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/blimp_clien... > > So why not create the IdentityProvider there as well, > base::WrapUnique<IdentityProvider>(new ProfileIdentityProvider( > SigninManagerFactory::GetForProfile(profile_), > ProfileOAuth2TokenServiceFactory::GetForProfile(profile_), > base::Closure())) > You can get profile the same way, Profile::FromBrowserContext(context). > > Am I missing something here? Yeah, that's probably the right way to do that :)
The CQ bit was checked by xingliu@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...
On 2016/11/03 01:45:37, xingliu wrote: > Thanks for the feedback, it's better to create IdentityProvider early on, filed > a bug for that. > > But it won't be done in this CL, this CL mainly targets to fix the crash bug > which blocks other people. Sounds good. Thanks for filing the bug and taking a look at it. lgtm again. :) > > https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... > File blimp/client/core/context/blimp_client_context_impl.cc (right): > > https://codereview.chromium.org/2470913005/diff/40001/blimp/client/core/conte... > blimp/client/core/context/blimp_client_context_impl.cc:150: void > BlimpClientContextImpl::SetDelegate(BlimpClientContextDelegate* delegate) { > On 2016/11/03 00:50:48, Khushal wrote: > > On 2016/11/03 00:33:31, xingliu wrote: > > > On 2016/11/03 00:13:35, Khushal wrote: > > > > This is being called with a null delegate from its dtor. See > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/chrome_blim... > > > > > > > > So what you want to is: > > > > DCHECK(!delegate_ || !delegate), basically one of them should be null. > Then, > > > > > > > > if (delegate_) { > > > > assignment_fetcher_ = nullptr; > > > > } > > > > delegate_ = delegate; > > > > if (delegate_) { > > > > assignment_fetcher_ = // Create the fetcher. > > > > } > > > > > > Sorry I didn't notice in dtor it sets to nullptr. > > > But assignment_fetcher_can probably out live the delegate? > > > > I just looked at that class, and yes, it does look like the fetcher doesn't > have > > anything to do with the delegate. But in that case, I'm super confused why we > > have a delegate callback for creating the IdentitySource. Why don't we pass it > > in the context's ctor, like the other things tied to the Profile when creating > > it. > > For instance, the PrefService is passed at that time here: > > > https://cs.chromium.org/chromium/src/chrome/browser/android/blimp/blimp_clien... > > > > So why not create the IdentityProvider there as well, > > base::WrapUnique<IdentityProvider>(new ProfileIdentityProvider( > > SigninManagerFactory::GetForProfile(profile_), > > ProfileOAuth2TokenServiceFactory::GetForProfile(profile_), > > base::Closure())) > > You can get profile the same way, Profile::FromBrowserContext(context). > > > > Am I missing something here? > > Yeah, that's probably the right way to do that :)
The CQ bit was unchecked by xingliu@chromium.org
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2470913005/#ps60001 (title: "Polish the SetDelegate call.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. +1 on doing it in the constructor. We had talked about it. You might have to make sure certain dependencies are built properly. nyquist@ did some work there with factory dependency ordering.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by xingliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix the blimp settings crash. The assignment fetcher is not initialized when the settings jni bridge needs it, which causes the crash. To reproduce the bug, we need to clear adb_chrome_public_command_line. Run: build/android/adb_chrome_public_command_line "" Intialization detail: Since assignment fetcher and identity source both need identity provider, which is a service created from BlimpClientContextDelegate, they can't be created in the ctor of BlimpClientContextImpl. With another CL: https://codereview.chromium.org/2471733002/ We should be able to run local server with client_engine_integration script, but currently there might be other bugs that local client still can't receive messages from local engine. BUG=661322 ========== to ========== Fix the blimp settings crash. The assignment fetcher is not initialized when the settings jni bridge needs it, which causes the crash. To reproduce the bug, we need to clear adb_chrome_public_command_line. Run: build/android/adb_chrome_public_command_line "" Intialization detail: Since assignment fetcher and identity source both need identity provider, which is a service created from BlimpClientContextDelegate, they can't be created in the ctor of BlimpClientContextImpl. With another CL: https://codereview.chromium.org/2471733002/ We should be able to run local server with client_engine_integration script, but currently there might be other bugs that local client still can't receive messages from local engine. BUG=661322 Committed: https://crrev.com/6a658f84fc845447726b451b2e26dbd043cea5c6 Cr-Commit-Position: refs/heads/master@{#429618} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a658f84fc845447726b451b2e26dbd043cea5c6 Cr-Commit-Position: refs/heads/master@{#429618} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
