file-handling: do not update App's origin trial expiry if file handling is force enabled

This resolves an race condition when an App is session restored, and its
file handling is being force enabled.

FileHandlingExpiry service involves IPC calls, whose replies are
received after ForceEnableFileHandlingOriginTrial. This causes
FileHandlerManager to disable the App's file handling, if the App does
not hold a valid origin trial token (even though File Handling was force
enabled, and should not require a origin trial token).

Fixed: 1064814
Change-Id: I294070a5cac4645784ff1d773c6f7845772b45b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2143773
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758756}

chrome/browser/ui/web_applications/web_app_file_handling_browsertest.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
+#include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/strings/strcat.h"
@@ -53,11 +55,23 @@ class FakeFileHandlingExpiryService
 
   void RequestOriginTrialExpiryTime(
       RequestOriginTrialExpiryTimeCallback callback) override {
+    if (before_reply_callback_) {
+      std::move(before_reply_callback_).Run();
+    }
+
     std::move(callback).Run(expiry_time_);
   }
 
+  // Set a callback to be called before FileHandlingExpiry interface replies
+  // the expiry time. Useful for testing inflight IPC.
+  void SetBeforeReplyCallback(base::RepeatingClosure before_reply_callback) {
+    before_reply_callback_ = before_reply_callback;
+  }
+
  private:
   base::Time expiry_time_;
+  RequestOriginTrialExpiryTimeCallback callback_;
+  base::RepeatingClosure before_reply_callback_;
   mojo::AssociatedReceiver<blink::mojom::FileHandlingExpiry> receiver_{this};
 };
 
@@ -397,6 +411,66 @@ IN_PROC_BROWSER_TEST_P(WebAppFileHandlingOriginTrialBrowserTest,
   EXPECT_EQ(nullptr, file_handler_manager().GetEnabledFileHandlers(app_id()));
 }
 
+IN_PROC_BROWSER_TEST_P(WebAppFileHandlingOriginTrialBrowserTest,
+                       ForceEnabledFileHandling_IgnoreExpiryTimeUpdate) {
+  InstallFileHandlingPWA();
+  SetUpInterceptorNavigateToAppAndMaybeWait();
+
+  EXPECT_FALSE(file_handler_manager().IsFileHandlingForceEnabled(app_id()));
+
+  // Force enables file handling.
+  file_handler_manager().ForceEnableFileHandlingOriginTrial(app_id());
+  EXPECT_TRUE(file_handler_manager().IsFileHandlingForceEnabled(app_id()));
+
+  // Update origin trial expiry time from the App's WebContents.
+  base::RunLoop loop;
+  file_handler_manager().SetOnFileHandlingExpiryUpdatedForTesting(
+      loop.QuitClosure());
+  file_handling_expiry().SetExpiryTime(base::Time());
+  file_handler_manager().MaybeUpdateFileHandlingOriginTrialExpiry(
+      web_contents(), app_id());
+  loop.Run();
+
+  // Force enabled file handling should not be updated by the expiry time in
+  // App's WebContents (i.e. origin trial token expiry).
+  EXPECT_TRUE(file_handler_manager().IsFileHandlingForceEnabled(app_id()));
+  EXPECT_TRUE(file_handler_manager().IsFileHandlingAPIAvailable(app_id()));
+  EXPECT_TRUE(file_handler_manager().GetEnabledFileHandlers(app_id()));
+}
+
+IN_PROC_BROWSER_TEST_P(WebAppFileHandlingOriginTrialBrowserTest,
+                       ForceEnabledFileHandling_IgnoreExpiryTimeInflightIPC) {
+  InstallFileHandlingPWA();
+  SetUpInterceptorNavigateToAppAndMaybeWait();
+
+  EXPECT_FALSE(file_handler_manager().IsFileHandlingForceEnabled(app_id()));
+
+  // Request to update origin trial expiry time from the App's WebContents, and
+  // force enables file handling origin trial before the expiry time reply is
+  // received.
+  base::RunLoop loop;
+  file_handler_manager().SetOnFileHandlingExpiryUpdatedForTesting(
+      loop.QuitClosure());
+  file_handling_expiry().SetExpiryTime(base::Time());
+  file_handling_expiry().SetBeforeReplyCallback(
+      base::BindLambdaForTesting([&]() {
+        EXPECT_FALSE(
+            file_handler_manager().IsFileHandlingForceEnabled(app_id()));
+        file_handler_manager().ForceEnableFileHandlingOriginTrial(app_id());
+      }));
+
+  EXPECT_FALSE(file_handler_manager().IsFileHandlingForceEnabled(app_id()));
+  file_handler_manager().MaybeUpdateFileHandlingOriginTrialExpiry(
+      web_contents(), app_id());
+  loop.Run();
+
+  // Force enabled file handling should not be updated by the inflight expiry
+  // time IPC.
+  EXPECT_TRUE(file_handler_manager().IsFileHandlingForceEnabled(app_id()));
+  EXPECT_TRUE(file_handler_manager().IsFileHandlingAPIAvailable(app_id()));
+  EXPECT_TRUE(file_handler_manager().GetEnabledFileHandlers(app_id()));
+}
+
 namespace {
 static constexpr char kBaseDataDir[] = "chrome/test/data/web_app_file_handling";
 

chrome/browser/web_applications/components/file_handler_manager.cc

@@ -108,9 +108,17 @@ void FileHandlerManager::DisableAndUnregisterOsFileHandlers(
   UnregisterFileHandlersWithOs(app_id, profile());
 }
 
-void FileHandlerManager::UpdateFileHandlingOriginTrialExpiry(
+void FileHandlerManager::MaybeUpdateFileHandlingOriginTrialExpiry(
     content::WebContents* web_contents,
     const AppId& app_id) {
+  // If an App has force enabled file handling, there is no need to check its
+  // WebContents.
+  if (IsFileHandlingForceEnabled(app_id)) {
+    if (on_file_handling_expiry_updated_for_testing_)
+      on_file_handling_expiry_updated_for_testing_.Run();
+    return;
+  }
+
   mojo::AssociatedRemote<blink::mojom::FileHandlingExpiry> expiry_service;
   web_contents->GetMainFrame()->GetRemoteAssociatedInterfaces()->GetInterface(
       &expiry_service);
@@ -167,7 +175,15 @@ void FileHandlerManager::OnOriginTrialExpiryTimeReceived(
     mojo::AssociatedRemote<blink::mojom::FileHandlingExpiry> /*interface*/,
     const AppId& app_id,
     base::Time expiry_time) {
-  UpdateFileHandlersForOriginTrialExpiryTime(app_id, expiry_time);
+  // Updates the expiry time, if file handling is enabled by origin trial
+  // tokens. If an App has force enabled file handling, it might not have expiry
+  // time associated with it.
+  if (!IsFileHandlingForceEnabled(app_id)) {
+    UpdateFileHandlersForOriginTrialExpiryTime(app_id, expiry_time);
+  }
+
+  if (on_file_handling_expiry_updated_for_testing_)
+    on_file_handling_expiry_updated_for_testing_.Run();
 }
 
 void FileHandlerManager::UpdateFileHandlersForOriginTrialExpiryTime(
@@ -189,9 +205,6 @@ void FileHandlerManager::UpdateFileHandlersForOriginTrialExpiryTime(
   } else if (file_handlers_enabled) {
     DisableAndUnregisterOsFileHandlers(app_id);
   }
-
-  if (on_file_handling_expiry_updated_for_testing_)
-    on_file_handling_expiry_updated_for_testing_.Run();
 }
 
 void FileHandlerManager::DisableAutomaticFileHandlerCleanupForTesting() {
@@ -264,4 +277,12 @@ const base::Optional<GURL> FileHandlerManager::GetMatchingFileHandlerURL(
   return base::nullopt;
 }
 
+bool FileHandlerManager::IsFileHandlingForceEnabled(const AppId& app_id) {
+  double pref_expiry_time =
+      GetDoubleWebAppPref(profile()->GetPrefs(), app_id,
+                          kFileHandlingOriginTrialExpiryTime)
+          .value_or(0);
+  return pref_expiry_time == kMaxOriginTrialExpiryTime;
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/file_handler_manager.h

@@ -74,9 +74,11 @@ class FileHandlerManager : public AppRegistrarObserver {
   void DisableAndUnregisterOsFileHandlers(const AppId& app_id);
 
   // Updates the file handling origin trial expiry timer based on a currently
-  // open instance of the site.
-  void UpdateFileHandlingOriginTrialExpiry(content::WebContents* web_contents,
-                                           const AppId& app_id);
+  // open instance of the site. This will not update the expiry timer if
+  // |app_id| has force enabled file handling origin trial.
+  void MaybeUpdateFileHandlingOriginTrialExpiry(
+      content::WebContents* web_contents,
+      const AppId& app_id);
 
   // Force enables File Handling origin trial. This will register the App's file
   // handlers even if the App does not have a valid origin trial token.
@@ -86,6 +88,9 @@ class FileHandlerManager : public AppRegistrarObserver {
   // App's file handlers.
   void DisableForceEnabledFileHandlingOriginTrial(const AppId& app_id);
 
+  // Returns whether App's file handling is force enabled.
+  bool IsFileHandlingForceEnabled(const AppId& app_id);
+
   // Gets all enabled file handlers for |app_id|. |nullptr| if the app has no
   // enabled file handlers. Note: The lifetime of the file handlers are tied to
   // the app they belong to.

chrome/browser/web_applications/web_app_tab_helper.cc

@@ -106,11 +106,9 @@ void WebAppTabHelper::DOMContentLoaded(
   if (app_id_.empty())
     return;
 
-  // Ordinary Web Apps need to hold valid origin trial tokens to continue using
-  // File Handling API.
-  if (provider_->system_web_app_manager().IsSystemWebApp(app_id_))
-    return;
-  provider_->file_handler_manager().UpdateFileHandlingOriginTrialExpiry(
+  // There is no way to reliably know if |app_id_| is for a System Web App
+  // during startup, so we always call MaybeUpdateFileHandlingOriginTrialExpiry.
+  provider_->file_handler_manager().MaybeUpdateFileHandlingOriginTrialExpiry(
       web_contents(), app_id_);
 }
 
@@ -138,17 +136,8 @@ void WebAppTabHelper::OnWebAppInstalled(const AppId& installed_app_id) {
 
   SetAppId(app_id);
 
-  // Ordinary Web Apps need valid origin trial tokens to use File Handling API.
-  // When an App is installed, record its origin trial expiry time.
-  //
   // TODO(crbug.com/1053371): Clean up where we install file handlers.
-  // The following check is not necessary. This tab helper is not attached to
-  // the WebContents we used to install the WebApp. When a System Web App is
-  // installed, there is no associated tab and we early return at `app_id !=
-  // installed_app_id`.
-  if (provider_->system_web_app_manager().IsSystemWebApp(installed_app_id))
-    return;
-  provider_->file_handler_manager().UpdateFileHandlingOriginTrialExpiry(
+  provider_->file_handler_manager().MaybeUpdateFileHandlingOriginTrialExpiry(
       web_contents(), installed_app_id);
 }