PerFrame Translation fix subframe issues with completing translation

This includes 3 fixes determined from turning on amp carousel page
translation:
  1. Only try to translate subframes that are visible.
  2. Adds retry count to CheckTranslateStatus (to break rare loop).
  3. Fixes overall translate error code to the one for the main frame.

Note that these changes are in classes only used with Feature flag so
no risk to mainline regressions.

Bug: 1084262
Change-Id: I03541b9ab78813b2ace55e9b5e0379a8e952f8d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211042
Commit-Queue: Doug Arnett <dougarnett@chromium.org>
Reviewed-by: Megan Jablonski <megjablon@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771216}

chrome/browser/translate/translate_manager_browsertest.cc

@@ -101,7 +101,7 @@ static const char kTestScriptIdenticalLanguages[] =
     "})();"
     "cr.googleTranslate.onTranslateElementLoad();";
 
-static const char kTestScriptTimeout[] =
+static const char kTestScriptAvailableTimeout[] =
     "var google = {};"
     "google.translate = (function() {"
     "  return {"
@@ -116,6 +116,31 @@ static const char kTestScriptTimeout[] =
     "})();"
     "cr.googleTranslate.onTranslateElementLoad();";
 
+static const char kTestScriptTranslateTimeout[] =
+    "var google = {};"
+    "google.translate = (function() {"
+    "  return {"
+    "    TranslateService: function() {"
+    "      return {"
+    "        isAvailable : function() {"
+    "          return true;"
+    "        },"
+    "        restore : function() {"
+    "          return;"
+    "        },"
+    "        getDetectedLanguage : function() {"
+    "          return \"fr\";"
+    "        },"
+    "        translatePage : function(originalLang, targetLang,"
+    "                                 onTranslateProgress) {"
+    "          onTranslateProgress(33, false, 0);"
+    "        }"
+    "      };"
+    "    }"
+    "  };"
+    "})();"
+    "cr.googleTranslate.onTranslateElementLoad();";
+
 static const char kTestScriptUnexpectedScriptError[] =
     "var google = {};"
     "google.translate = (function() {"
@@ -872,7 +897,7 @@ IN_PROC_BROWSER_TEST_F(TranslateManagerBrowserTest,
 // Test the checks translate lib never gets ready and throws timeout.
 IN_PROC_BROWSER_TEST_F(TranslateManagerBrowserTest,
                        PageTranslationTimeoutError) {
-  SetTranslateScript(kTestScriptTimeout);
+  SetTranslateScript(kTestScriptAvailableTimeout);
 
   ChromeTranslateClient* chrome_translate_client = GetChromeTranslateClient();
 
@@ -1310,41 +1335,6 @@ IN_PROC_BROWSER_TEST_F(TranslateManagerWithSubFrameSupportBrowserTest,
                                1);
 }
 
-// Test that the translation was successful in an about:blank page.
-// This is a regression test for https://crbug.com/943685.
-IN_PROC_BROWSER_TEST_F(TranslateManagerWithSubFrameSupportBrowserTest,
-                       PageTranslationAboutBlank) {
-  SetTranslateScript(kTestValidScript);
-  AddTabAtIndex(0, GURL(embedded_test_server()->GetURL("/french_page.html")),
-                ui::PAGE_TRANSITION_TYPED);
-  ResetObserver();
-
-  // Open a pop-up window and leave it at the initial about:blank URL.
-  content::WebContentsAddedObserver popup_observer;
-  ASSERT_TRUE(
-      content::ExecJs(browser()->tab_strip_model()->GetActiveWebContents(),
-                      "window.open('about:blank', 'popup')"));
-  content::WebContents* popup = popup_observer.GetWebContents();
-
-  // A round-trip to the renderer process helps avoid a race where the
-  // browser-side translate structures are not yet ready for the translate call.
-  EXPECT_EQ("ping", content::EvalJs(popup, "'ping'"));
-
-  // Translate the about:blank page.
-  ChromeTranslateClient* chrome_translate_client =
-      ChromeTranslateClient::FromWebContents(popup);
-  TranslateManager* manager = chrome_translate_client->GetTranslateManager();
-  manager->TranslatePage("fr", "en", true);
-
-  // Verify that the crash from https://crbug.com/943685 didn't happen.
-  EXPECT_EQ("still alive", content::EvalJs(popup, "'still alive'"));
-
-  // Wait for translation to finish and verify it was successful.
-  WaitUntilPageTranslated();
-  EXPECT_FALSE(chrome_translate_client->GetLanguageState().translation_error());
-  EXPECT_EQ(TranslateErrors::NONE, GetPageTranslatedResult());
-}
-
 // Test that hrefTranslate is propagating properly
 IN_PROC_BROWSER_TEST_F(TranslateManagerWithSubFrameSupportBrowserTest,
                        HrefTranslateSuccess) {
@@ -1655,8 +1645,8 @@ IN_PROC_BROWSER_TEST_F(TranslateManagerWithSubFrameSupportBrowserTest,
 
   ChromeTranslateClient* chrome_translate_client = GetChromeTranslateClient();
 
-  // Open a new tab with about:blank page.
-  AddTabAtIndex(0, GURL("about:blank"), ui::PAGE_TRANSITION_TYPED);
+  AddTabAtIndex(0, GURL(embedded_test_server()->GetURL("/french_page.html")),
+                ui::PAGE_TRANSITION_TYPED);
   ResetObserver();
   chrome_translate_client = GetChromeTranslateClient();
   TranslateManager* manager = chrome_translate_client->GetTranslateManager();
@@ -1701,8 +1691,39 @@ IN_PROC_BROWSER_TEST_F(TranslateManagerWithSubFrameSupportBrowserTest,
 
 // Test the checks translate lib never gets ready and throws timeout.
 IN_PROC_BROWSER_TEST_F(TranslateManagerWithSubFrameSupportBrowserTest,
-                       PageTranslationTimeoutError) {
-  SetTranslateScript(kTestScriptTimeout);
+                       PageTranslationAvailableTimeoutError) {
+  SetTranslateScript(kTestScriptAvailableTimeout);
+
+  ChromeTranslateClient* chrome_translate_client = GetChromeTranslateClient();
+
+  // Open a new tab with a page in French.
+  AddTabAtIndex(0, GURL(embedded_test_server()->GetURL("/french_page.html")),
+                ui::PAGE_TRANSITION_TYPED);
+  ResetObserver();
+  chrome_translate_client = GetChromeTranslateClient();
+  WaitUntilLanguageDetermined();
+
+  EXPECT_EQ("fr",
+            chrome_translate_client->GetLanguageState().original_language());
+
+  // Translate the page through TranslateManager.
+  TranslateManager* manager = chrome_translate_client->GetTranslateManager();
+  manager->TranslatePage(
+      chrome_translate_client->GetLanguageState().original_language(), "en",
+      true);
+
+  WaitUntilPageTranslated();
+
+  EXPECT_TRUE(chrome_translate_client->GetLanguageState().translation_error());
+  EXPECT_EQ(TranslateErrors::TRANSLATION_TIMEOUT, GetPageTranslatedResult());
+}
+
+// Test the checks translate operation status never resolves.
+// TODO(1064974): consolidate the common test logic here that is used between
+// several error type tests from different script inputs.
+IN_PROC_BROWSER_TEST_F(TranslateManagerWithSubFrameSupportBrowserTest,
+                       PageTranslationTranslateTimeoutError) {
+  SetTranslateScript(kTestScriptTranslateTimeout);
 
   ChromeTranslateClient* chrome_translate_client = GetChromeTranslateClient();
 

components/translate/content/browser/per_frame_content_translate_driver.cc

@@ -104,6 +104,7 @@ PerFrameContentTranslateDriver::PendingRequestStats::~PendingRequestStats() =
 void PerFrameContentTranslateDriver::PendingRequestStats::Clear() {
   frame_request_count = 0;
   main_frame_success = false;
+  main_frame_error = TranslateErrors::NONE;
   frame_success_count = 0;
   frame_errors.clear();
 }
@@ -154,6 +155,11 @@ void PerFrameContentTranslateDriver::TranslateFrame(
     const std::string& source_lang,
     const std::string& target_lang,
     content::RenderFrameHost* render_frame_host) {
+  if (render_frame_host->IsFrameDisplayNone() ||
+      !translate::IsTranslatableURL(render_frame_host->GetLastCommittedURL())) {
+    return;
+  }
+
   bool is_main_frame = (!render_frame_host->GetParent());
   mojo::AssociatedRemote<mojom::TranslateAgent> frame_agent;
   render_frame_host->GetRemoteAssociatedInterfaces()->GetInterface(
@@ -177,6 +183,11 @@ void PerFrameContentTranslateDriver::RevertTranslation(int page_seq_no) {
 
 void PerFrameContentTranslateDriver::RevertFrame(
     content::RenderFrameHost* render_frame_host) {
+  if (render_frame_host->IsFrameDisplayNone() ||
+      !translate::IsTranslatableURL(render_frame_host->GetLastCommittedURL())) {
+    return;
+  }
+
     mojo::AssociatedRemote<mojom::TranslateAgent> frame_agent;
     render_frame_host->GetRemoteAssociatedInterfaces()->GetInterface(
         &frame_agent);
@@ -409,10 +420,14 @@ void PerFrameContentTranslateDriver::OnFrameTranslated(
     }
   } else {
     stats_.frame_errors.push_back(error_type);
+    if (is_main_frame) {
+      stats_.main_frame_error = error_type;
+    }
   }
 
   if (--stats_.pending_request_count == 0) {
-    OnPageTranslated(cancelled, original_lang, translated_lang, error_type);
+    OnPageTranslated(cancelled, original_lang, translated_lang,
+                     stats_.main_frame_error);
     stats_.Report();
   }
 }

components/translate/content/browser/per_frame_content_translate_driver.h

@@ -78,6 +78,7 @@ class PerFrameContentTranslateDriver : public ContentTranslateDriver {
     bool main_frame_success = false;
     int frame_request_count = 0;
     int frame_success_count = 0;
+    TranslateErrors::Type main_frame_error = TranslateErrors::NONE;
     std::vector<TranslateErrors::Type> frame_errors;
   };
 

components/translate/content/renderer/per_frame_translate_agent.cc

@@ -54,6 +54,10 @@ const int kMaxTranslateInitCheckAttempts = 5;
 // finished.
 const int kTranslateStatusCheckDelayMs = 400;
 
+// The maximum number of times we'll check whether the translation has
+// finished.
+const int kMaxTranslateStatusCheckAttempts = 10;
+
 // Language name passed to the Translate element for it to detect the language.
 const char kAutoDetectionLanguage[] = "auto";
 
@@ -150,7 +154,6 @@ void PerFrameTranslateAgent::TranslateFrame(const std::string& translate_script,
 
 void PerFrameTranslateAgent::RevertTranslation() {
   if (!IsTranslateLibAvailable()) {
-    NOTREACHED();
     return;
   }
 
@@ -288,7 +291,8 @@ int64_t PerFrameTranslateAgent::ExecuteScriptAndGetIntegerResult(
 
 ////////////////////////////////////////////////////////////////////////////////
 // PerFrameTranslateAgent, private:
-void PerFrameTranslateAgent::CheckTranslateStatus() {
+void PerFrameTranslateAgent::CheckTranslateStatus(int check_count) {
+  DCHECK_LT(check_count, kMaxTranslateStatusCheckAttempts);
   // First check if there was an error.
   if (HasTranslationFailed()) {
     NotifyBrowserTranslationFailed(
@@ -329,13 +333,20 @@ void PerFrameTranslateAgent::CheckTranslateStatus() {
     return;
   }
 
-  // The translation is still pending, check again later.
+  // The translation is still pending, check again later unless we have tried
+  // many times already.
+  if (++check_count >= kMaxTranslateStatusCheckAttempts) {
+    NotifyBrowserTranslationFailed(TranslateErrors::TRANSLATION_TIMEOUT);
+    return;
+  }
+
+  // Check again later.
   render_frame()
       ->GetTaskRunner(blink::TaskType::kInternalTranslation)
       ->PostDelayedTask(
           FROM_HERE,
           base::BindOnce(&PerFrameTranslateAgent::CheckTranslateStatus,
-                         weak_method_factory_.GetWeakPtr()),
+                         weak_method_factory_.GetWeakPtr(), check_count),
           AdjustDelay(kTranslateStatusCheckDelayMs));
 }
 
@@ -372,7 +383,9 @@ void PerFrameTranslateAgent::TranslateFrameImpl(int try_count) {
       ExecuteScriptAndGetDoubleResult("cr.googleTranslate.loadTime"));
 
   if (!StartTranslation()) {
-    CheckTranslateStatus();
+    DCHECK(HasTranslationFailed());
+    NotifyBrowserTranslationFailed(
+        static_cast<translate::TranslateErrors::Type>(GetErrorCode()));
     return;
   }
   // Check the status of the translation.
@@ -381,7 +394,7 @@ void PerFrameTranslateAgent::TranslateFrameImpl(int try_count) {
       ->PostDelayedTask(
           FROM_HERE,
           base::BindOnce(&PerFrameTranslateAgent::CheckTranslateStatus,
-                         weak_method_factory_.GetWeakPtr()),
+                         weak_method_factory_.GetWeakPtr(), 0),
           AdjustDelay(kTranslateStatusCheckDelayMs));
 }
 

components/translate/content/renderer/per_frame_translate_agent.h

@@ -124,8 +124,9 @@ class PerFrameTranslateAgent : public content::RenderFrameObserver,
 
   // Checks if the current running page translation is finished or errored and
   // notifies the browser accordingly.  If the translation has not terminated,
-  // posts a task to check again later.
-  void CheckTranslateStatus();
+  // posts a task to check again later. |check_count| is used to limit the
+  // number of retries.
+  void CheckTranslateStatus(int check_count);
 
   // Called by TranslateFrame to do the actual translation.  |count| is used to
   // limit the number of retries.