Run LocalizeStrings() on plugin thread.

remoting/host/plugin/host_script_object.cc

 // Copyright (c) 2011 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 "remoting/host/plugin/host_script_object.h"
 
 #include "base/bind.h"
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
 #include "base/sys_string_conversions.h"
 #include "base/threading/platform_thread.h"
 #include "base/utf_string_conversions.h"
 #include "remoting/base/auth_token_util.h"
 #include "remoting/base/util.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
 #include "remoting/host/desktop_environment.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/host_key_pair.h"
 #include "remoting/host/in_memory_host_config.h"
 #include "remoting/host/plugin/policy_hack/nat_policy.h"
 #include "remoting/host/register_support_host_request.h"
 #include "remoting/host/support_access_verifier.h"
-#include "remoting/host/ui_strings.h"
 
 namespace remoting {
 
 // Supported Javascript interface:
 // readonly attribute string accessCode;
 // readonly attribute int accessCodeLifetime;
 // readonly attribute string client;
 // readonly attribute int state;
 //
 // state: {
@@ skipping 235 matching lines @@
     } else {
       SetException("SetProperty: unexpected type for property " +
                    property_name);
     }
     return false;
   }
 
   if (property_name == kAttrNameLocalizeString) {
     if (NPVARIANT_IS_OBJECT(*value)) {
       localize_func_ = NPVARIANT_TO_OBJECT(*value);
+      LocalizeStrings();

[Sergey Ulanov, 2011/08/29 21:22:05]

Can we call this from Connect()? JavaScript may not expect localization callback
to be called when it sets this property.

[Jamie, 2011/08/29 21:50:46]

I don't think that buys us anything, and I prefer to have things localized as
soon as they possibly can be so that it's out of the way and we don't need to
think about it any more.

[Sergey Ulanov, 2011/08/29 22:28:07]

What bothers me is that javascript interface looks really weird with this
change:
JS code just sets property localizeString to a method, and in result this method
is called before property is set.
Maybe we should replace this property with localize() method, and pass the
pointer to the localization function to that method?

[Lambros, 2011/08/29 23:24:59]

Done.

       return true;
     } else {
       SetException("SetProperty: unexpected type for property " +
                    property_name);
     }
     return false;
   }
 
   if (property_name == kAttrNameLogDebugInfo) {
     if (NPVARIANT_IS_OBJECT(*value)) {
@@ skipping 186 matching lines @@
 
   // Create the Host.
   LOG(INFO) << "Connecting with NAT state: " << nat_traversal_enabled_;
   host_ = ChromotingHost::Create(
       &host_context_, host_config_, desktop_environment_.get(),
       access_verifier.release(), nat_traversal_enabled_);
   host_->AddStatusObserver(this);
   host_->AddStatusObserver(register_request_.get());
   host_->set_it2me(true);
 
-  LocalizeStrings();
+  {
+    base::AutoLock auto_lock(ui_strings_lock_);
+    host_->SetUiStrings(ui_strings_);
+  }
 
   // Start the Host.
   host_->Start();
 
   OnStateChanged(kRequestedAccessCode);
   return;
 }
 
 bool HostNPScriptObject::Disconnect(const NPVariant* args,
                                     uint32_t arg_count,
@@ skipping 142 matching lines @@
   }
 }
 
 void HostNPScriptObject::SetException(const std::string& exception_string) {
   CHECK_EQ(base::PlatformThread::CurrentId(), np_thread_id_);
   g_npnetscape_funcs->setexception(parent_, exception_string.c_str());
   LOG(INFO) << exception_string;
 }
 
 void HostNPScriptObject::LocalizeStrings() {
-  UiStrings ui_strings;
+  DCHECK(plugin_message_loop_proxy_->BelongsToCurrentThread());

[Jamie, 2011/08/29 20:59:22]

Other similar checks in the same file use CHECK_EQ, so is DCHECK right here, or
would we be better off with CHECK?

[Lambros, 2011/08/29 21:21:42]

Removed.

[Sergey Ulanov, 2011/08/29 21:22:05]

IMO DCHECK is fine here. We should change this code to use DCHECK when checking
thread in other places too.

[Sergey Ulanov, 2011/08/29 22:30:27]

Please add it back. We do need this DCHECK to ensure that we localize on the
right thread (we wouldn't have this bug if we had a DCHECK here in the first
place).

[Lambros, 2011/08/29 23:24:59]

Done.

+
+  base::AutoLock auto_lock(ui_strings_lock_);

[Jamie, 2011/08/29 20:59:22]

I worry slightly about holding this lock while we're calling into JS. Might it
be cleaner to keep the temporary ui_strings local and assign it to the member
variable after localizing? That way, only the assignment needs to be mutexed.

[Lambros, 2011/08/29 21:21:42]

Done.

[Sergey Ulanov, 2011/08/29 21:22:05]

+1. If javascript sets the localization callback it would deadlock this code.

   string16 direction;
   LocalizeString("@@bidi_dir", &direction);
-  ui_strings.direction = UTF16ToUTF8(direction) == "rtl" ?
+  ui_strings_.direction = UTF16ToUTF8(direction) == "rtl" ?
       remoting::UiStrings::RTL : remoting::UiStrings::LTR;
-  LocalizeString(/*i18n-content*/"PRODUCT_NAME", &ui_strings.product_name);
+  LocalizeString(/*i18n-content*/"PRODUCT_NAME", &ui_strings_.product_name);
   LocalizeString(/*i18n-content*/"DISCONNECT_BUTTON",
-                 &ui_strings.disconnect_button_text);
+                 &ui_strings_.disconnect_button_text);
   LocalizeString(
 #if defined(OS_WIN)
       /*i18n-content*/"DISCONNECT_BUTTON_PLUS_SHORTCUT_WINDOWS",
 #elif defined(OS_MAC)
       /*i18n-content*/"DISCONNECT_BUTTON_PLUS_SHORTCUT_MAC_OS_X",
 #else
       /*i18n-content*/"DISCONNECT_BUTTON_PLUS_SHORTCUT_LINUX",
 #endif
-      &ui_strings.disconnect_button_text_plus_shortcut);
+      &ui_strings_.disconnect_button_text_plus_shortcut);
   LocalizeString(/*i18n-content*/"CONTINUE_PROMPT",
-                 &ui_strings.continue_prompt);
+                 &ui_strings_.continue_prompt);
   LocalizeString(/*i18n-content*/"CONTINUE_BUTTON",
-                 &ui_strings.continue_button_text);
+                 &ui_strings_.continue_button_text);
   LocalizeString(/*i18n-content*/"STOP_SHARING_BUTTON",
-                 &ui_strings.stop_sharing_button_text);
+                 &ui_strings_.stop_sharing_button_text);
   LocalizeString(/*i18n-content*/"MESSAGE_SHARED",
-                 &ui_strings.disconnect_message);
-
-  host_->SetUiStrings(ui_strings);
+                 &ui_strings_.disconnect_message);
 }
 
 bool HostNPScriptObject::LocalizeString(const char* tag, string16* result) {
   NPVariant args[2];
   STRINGZ_TO_NPVARIANT(tag, args[0]);
   NPVariant np_result;
   bool is_good = g_npnetscape_funcs->invokeDefault(
       plugin_, localize_func_.get(), &args[0], 1, &np_result);
   if (!is_good) {
     LOG(ERROR) << "Localization failed for " << tag;
@@ skipping 14 matching lines @@
                                                uint32_t argCount) {
   NPVariant np_result;
   bool is_good = g_npnetscape_funcs->invokeDefault(plugin_, func, args,
                                                    argCount, &np_result);
   if (is_good)
       g_npnetscape_funcs->releasevariantvalue(&np_result);
   return is_good;
 }
 
 }  // namespace remoting