Run LocalizeStrings() on plugin thread.

remoting/host/plugin/host_script_object.cc

Index: remoting/host/plugin/host_script_object.cc
diff --git a/remoting/host/plugin/host_script_object.cc b/remoting/host/plugin/host_script_object.cc
index d1f65af2b9e86fd0e1b00ea324e6a3b63ca4fbf0..d826cabe22ebf065abf0be8a0d08de67f86fafda 100644
--- a/remoting/host/plugin/host_script_object.cc
+++ b/remoting/host/plugin/host_script_object.cc
@@ -21,7 +21,6 @@
 #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 {
 
@@ -277,6 +276,7 @@ bool HostNPScriptObject::SetProperty(const std::string& property_name,
   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 " +
@@ -483,7 +483,10 @@ void HostNPScriptObject::FinishConnect(
   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();
@@ -646,14 +649,16 @@ void HostNPScriptObject::SetException(const std::string& 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",
@@ -662,17 +667,15 @@ void HostNPScriptObject::LocalizeStrings() {
 #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) {