Fix the blimp settings crash.

blimp/client/core/context/blimp_client_context_impl.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "blimp/client/core/context/blimp_client_context_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 143 matching lines @@
 std::unique_ptr<BlimpContents> BlimpClientContextImpl::CreateBlimpContents(
     gfx::NativeWindow window) {
   std::unique_ptr<BlimpContents> blimp_contents =
       blimp_contents_manager_->CreateBlimpContents(window);
   if (blimp_contents)
     delegate_->AttachBlimpContentsHelpers(blimp_contents.get());
   return blimp_contents;
 }
 
 void BlimpClientContextImpl::Connect() {
-  if (!assignment_fetcher_) {
-    assignment_fetcher_ = base::MakeUnique<AssignmentFetcher>(
-        io_thread_task_runner_, file_thread_task_runner_,
-        delegate_->CreateIdentityProvider(), GetAssignerURL(),
-        base::Bind(&BlimpClientContextImpl::OnAssignmentReceived,
-                   weak_factory_.GetWeakPtr()),
-        base::Bind(&BlimpClientContextDelegate::OnAuthenticationError,
-                   base::Unretained(delegate_)));
-  }
+  if (!assignment_fetcher_)
+    CreateAssignmentFetcher();
+
   assignment_fetcher_->Fetch();
 }
 
 void BlimpClientContextImpl::ConnectWithAssignment(
     const Assignment& assignment) {
   io_thread_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&ClientNetworkComponents::ConnectWithAssignment,
                  base::Unretained(net_components_.get()), assignment));
 }
 
 std::unordered_map<std::string, std::string>
 BlimpClientContextImpl::CreateFeedbackData() {
   return CreateBlimpFeedbackData(blimp_contents_manager_.get());
 }
 
 IdentitySource* BlimpClientContextImpl::GetIdentitySource() {
+  if (!assignment_fetcher_)
+    CreateAssignmentFetcher();
+
   return assignment_fetcher_->GetIdentitySource();
 }
 
 ConnectionStatus* BlimpClientContextImpl::GetConnectionStatus() {
   return &connection_status_;
 }
 
 GURL BlimpClientContextImpl::GetAssignerURL() {
   return GURL(kDefaultAssignerUrl);
 }
@@ skipping 46 matching lines @@
   tab_control_feature_->set_outgoing_message_processor(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kTabControl,
                                             tab_control_feature_.get()));
 }
 
 void BlimpClientContextImpl::DropConnection() {
   io_thread_task_runner_->PostTask(
       FROM_HERE, base::Bind(&DropConnectionOnIOThread, net_components_.get()));
 }
 
+void BlimpClientContextImpl::CreateAssignmentFetcher() {
+  if (assignment_fetcher_)

[Khushal, 2016/11/02 23:45:45]

nit: Why do the if check at the call sites if you're doing it here anyway?

+    return;
+
+  // |assignment_fetcher_| needs services from BlimpClientContextDelegate, so
+  // it can't be created in the constructor.
+  DCHECK(delegate_);
+  assignment_fetcher_ = base::MakeUnique<AssignmentFetcher>(

[Khushal, 2016/11/02 21:29:56]

Can we create this in SetDelegate instead then? Rather than having the if check
everywhere?

[xingliu, 2016/11/02 23:26:52]

@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.

[Khushal, 2016/11/02 23:45:45]

Definitely can't do without a |delegate_|, you need it for the
|assignment_fetcher_| for instance. :)
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.

[xingliu, 2016/11/03 00:08:04]

Yeah, agree.
Put it into SetDelegate. Since the delegate now is always needed.

+      io_thread_task_runner_, file_thread_task_runner_,
+      delegate_->CreateIdentityProvider(), GetAssignerURL(),
+      base::Bind(&BlimpClientContextImpl::OnAssignmentReceived,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&BlimpClientContextDelegate::OnAuthenticationError,
+                 base::Unretained(delegate_)));
+}
+
 void BlimpClientContextImpl::OnImageDecodeError() {
   // Currently we just drop the connection on image decoding error.
   DropConnection();
 }
 
 void BlimpClientContextImpl::OnConnected() {
   if (delegate_) {
     delegate_->OnConnected();
   }
 }
 
 void BlimpClientContextImpl::OnDisconnected(int result) {
   if (delegate_) {
     if (result >= 0) {
       delegate_->OnEngineDisconnected(result);
     } else {
       delegate_->OnNetworkDisconnected(result);
     }
   }
 }
 
 }  // namespace client
 }  // namespace blimp