Skip to content

Commit

Permalink
dPWA: remove File Handling origin trial code.
Browse files Browse the repository at this point in the history 
The code is flawed: in particular, it will re-enable/register file
handlers if it finds an origin trial is enabled, regardless of whether
the user has chosen to disable file handling for an app. (See
WebAppFileHandlerManager::UpdateFileHandlersForOriginTrialExpiryTime())

The code is deleted rather than fixed because the origin trial has
expired and will not be renewed.

Chrome OS system apps can use this origin trial to enable file
handling. In particular, the Files SWA does this. This functionality
is replaced by just enabling FH for all system web apps.

Follow-on cleanups include:
* removing OT code from FileManagerSystemAppDelegate
* removing FileHandlingExpiry from Blink

Bug: none
Change-Id: Ic2e25d3f2f20afa426d6cd004f27faaef4defbdc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3264354
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Phillis Tang <phillis@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#939970}
  • Loading branch information
Evan Stade authored and Chromium LUCI CQ committed Nov 9, 2021
1 parent 5f011fc commit a9d44e5
Show file tree
Hide file tree
Showing 8 changed files with 76 additions and 618 deletions.
473 changes: 62 additions & 411 deletions chrome/browser/ui/web_applications/web_app_file_handling_browsertest.cc
View file

Large diffs are not rendered by default.

21 changes: 0 additions & 21 deletions chrome/browser/web_applications/os_integration_manager.cc
View file
Expand Up @@ -297,27 +297,6 @@ const absl::optional<GURL> OsIntegrationManager::GetMatchingFileHandlerURL(
return file_handler_manager_->GetMatchingFileHandlerURL(app_id, launch_files);
}

void OsIntegrationManager::MaybeUpdateFileHandlingOriginTrialExpiry(
content::WebContents* web_contents,
const AppId& app_id) {
DCHECK(file_handler_manager_);
return file_handler_manager_->MaybeUpdateFileHandlingOriginTrialExpiry(
web_contents, app_id);
}

void OsIntegrationManager::ForceEnableFileHandlingOriginTrial(
const AppId& app_id) {
DCHECK(file_handler_manager_);
return file_handler_manager_->ForceEnableFileHandlingOriginTrial(app_id);
}

void OsIntegrationManager::DisableForceEnabledFileHandlingOriginTrial(
const AppId& app_id) {
DCHECK(file_handler_manager_);
return file_handler_manager_->DisableForceEnabledFileHandlingOriginTrial(
app_id);
}

absl::optional<GURL> OsIntegrationManager::TranslateProtocolUrl(
const AppId& app_id,
const GURL& protocol_url) {
Expand Down
9 changes: 0 additions & 9 deletions chrome/browser/web_applications/os_integration_manager.h
View file
Expand Up @@ -27,10 +27,6 @@

class Profile;

namespace content {
class WebContents;
}

namespace web_app {

class FakeOsIntegrationManager;
Expand Down Expand Up @@ -143,11 +139,6 @@ class OsIntegrationManager {
const absl::optional<GURL> GetMatchingFileHandlerURL(
const AppId& app_id,
const std::vector<base::FilePath>& launch_files);
void MaybeUpdateFileHandlingOriginTrialExpiry(
content::WebContents* web_contents,
const AppId& app_id);
void ForceEnableFileHandlingOriginTrial(const AppId& app_id);
void DisableForceEnabledFileHandlingOriginTrial(const AppId& app_id);

// Proxy calls for WebAppProtocolHandlerManager.
virtual absl::optional<GURL> TranslateProtocolUrl(const AppId& app_id,
Expand Down
22 changes: 4 additions & 18 deletions chrome/browser/web_applications/system_web_apps/system_web_app_manager.cc
View file
Expand Up @@ -103,10 +103,6 @@ namespace web_app {

namespace {

// Copy the origin trial name from runtime_enabled_features.json5, to avoid
// complex dependencies.
const char kFileHandlingOriginTrial[] = "FileHandling";

// Number of attempts to install a given version & locale of the SWAs before
// bailing out.
const int kInstallFailureAttempts = 3;
Expand Down Expand Up @@ -419,13 +415,6 @@ const std::vector<std::string>* SystemWebAppManager::GetEnabledOriginTrials(
return &iter_trials->second;
}

bool SystemWebAppManager::AppHasFileHandlingOriginTrial(
const SystemWebAppDelegate* system_app) {
const std::vector<std::string>* trials =
GetEnabledOriginTrials(system_app, system_app->GetInstallUrl());
return trials && base::Contains(*trials, kFileHandlingOriginTrial);
}

void SystemWebAppManager::OnReadyToCommitNavigation(
const AppId& app_id,
content::NavigationHandle* navigation_handle) {
Expand Down Expand Up @@ -595,13 +584,10 @@ void SystemWebAppManager::OnAppsSynchronized(
if (!app_id)
continue;

if (AppHasFileHandlingOriginTrial(it.second.get())) {
os_integration_manager_->ForceEnableFileHandlingOriginTrial(
app_id.value());
} else {
os_integration_manager_->DisableForceEnabledFileHandlingOriginTrial(
app_id.value());
}
InstallOsHooksOptions options;
options.os_hooks[OsHookType::kFileHandlers] = true;
os_integration_manager_->InstallOsHooks(*app_id, base::DoNothing(),
/*web_app_info=*/nullptr, options);
}

const base::TimeDelta install_duration =
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/web_applications/system_web_apps/system_web_app_manager.h
View file
Expand Up @@ -170,8 +170,6 @@ class SystemWebAppManager {
const SystemWebAppDelegate* system_app,
const GURL& url) const;

bool AppHasFileHandlingOriginTrial(const SystemWebAppDelegate* system_app);

void StopBackgroundTasks();

void OnAppsSynchronized(
Expand Down
121 changes: 10 additions & 111 deletions chrome/browser/web_applications/web_app_file_handler_manager.cc
View file
Expand Up @@ -20,26 +20,13 @@
#include "components/services/app_service/public/cpp/file_handler.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/web_contents.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/mojom/web_launch/file_handling_expiry.mojom.h"

namespace web_app {

namespace {
// Use a large double that can be safely saved in prefs, and be safely
// represented in JS timestamp (milliseconds from epoch). base::Time::Max() does
// not work here, it returns +Infinity (which is invalid and can not be
// represented in JSON).
//
// This value is `floor((2^53 - 1) / 1000)` because base::Time::FromDoubleT()
// accepts time offset in seconds. In reality, it means 287396-10-12 08:58:59
// UTC, which is a long distant future (long after File Handling goes out of
// origin trial or be deprecated).
//
// Do not change this value, because it is persisted to disk.
const double kMaxOriginTrialExpiryTime = 9007199254740;

// Used to enable running tests on platforms that don't support file handling
// icons.
Expand Down Expand Up @@ -84,12 +71,6 @@ void WebAppFileHandlerManager::SetIconsSupportedByOsForTesting(bool value) {
g_icons_supported_by_os_override = value;
}

void WebAppFileHandlerManager::SetOnFileHandlingExpiryUpdatedForTesting(
base::RepeatingCallback<void()> on_file_handling_expiry_updated) {
on_file_handling_expiry_updated_for_testing_ =
on_file_handling_expiry_updated;
}

void WebAppFileHandlerManager::EnableAndRegisterOsFileHandlers(
const AppId& app_id) {
if (!IsFileHandlingAPIAvailable(app_id))
Expand Down Expand Up @@ -153,48 +134,6 @@ void WebAppFileHandlerManager::DisableAndUnregisterOsFileHandlers(
#endif
}

void WebAppFileHandlerManager::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);
DCHECK(expiry_service);

auto* raw = expiry_service.get();

// Here we need to pass the |expiry_service| Mojom remote interface, so it is
// not destroyed before we get a reply.
raw->RequestOriginTrialExpiryTime(base::BindOnce(
&WebAppFileHandlerManager::OnOriginTrialExpiryTimeReceived,
weak_ptr_factory_.GetWeakPtr(), std::move(expiry_service), app_id));
}

void WebAppFileHandlerManager::ForceEnableFileHandlingOriginTrial(
const AppId& app_id) {
UpdateFileHandlersForOriginTrialExpiryTime(
app_id, base::Time::FromDoubleT(kMaxOriginTrialExpiryTime));
}

void WebAppFileHandlerManager::DisableForceEnabledFileHandlingOriginTrial(
const AppId& app_id) {
double pref_expiry_time =
GetDoubleWebAppPref(profile_->GetPrefs(), app_id,
kFileHandlingOriginTrialExpiryTime)
.value_or(0);
if (pref_expiry_time == kMaxOriginTrialExpiryTime) {
UpdateFileHandlersForOriginTrialExpiryTime(app_id, base::Time());
}
}

const apps::FileHandlers* WebAppFileHandlerManager::GetEnabledFileHandlers(
const AppId& app_id) {
if (AreFileHandlersEnabled(app_id) && IsFileHandlingAPIAvailable(app_id) &&
Expand All @@ -205,12 +144,16 @@ const apps::FileHandlers* WebAppFileHandlerManager::GetEnabledFileHandlers(
}

bool WebAppFileHandlerManager::IsFileHandlingAPIAvailable(const AppId& app_id) {
double pref_expiry_time =
GetDoubleWebAppPref(profile_->GetPrefs(), app_id,
kFileHandlingOriginTrialExpiryTime)
.value_or(0);
return base::FeatureList::IsEnabled(blink::features::kFileHandlingAPI) ||
base::Time::FromDoubleT(pref_expiry_time) >= base::Time::Now();
if (base::FeatureList::IsEnabled(blink::features::kFileHandlingAPI))
return true;

// May be null in unit tests.
if (registrar_) {
const WebApp* web_app = registrar_->GetAppById(app_id);
return web_app && web_app->IsSystemApp();
}

return false;
}

bool WebAppFileHandlerManager::AreFileHandlersEnabled(
Expand All @@ -225,42 +168,6 @@ bool WebAppFileHandlerManager::IconsEnabled() {
base::FeatureList::IsEnabled(blink::features::kFileHandlingIcons);
}

void WebAppFileHandlerManager::OnOriginTrialExpiryTimeReceived(
mojo::AssociatedRemote<blink::mojom::FileHandlingExpiry> /*interface*/,
const AppId& app_id,
base::Time 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 WebAppFileHandlerManager::UpdateFileHandlersForOriginTrialExpiryTime(
const AppId& app_id,
const base::Time& expiry_time) {
web_app::UpdateDoubleWebAppPref(profile_->GetPrefs(), app_id,
kFileHandlingOriginTrialExpiryTime,
expiry_time.ToDoubleT());
// Only enable/disable file handlers if the state is changing, as
// enabling/disabling is a potentially expensive operation (it may involve
// creating an app shim, and will almost certainly involve IO).
const bool file_handlers_enabled = AreFileHandlersEnabled(app_id);

// If the trial is valid, ensure the file handlers are enabled.
// Otherwise disable them.
if (IsFileHandlingAPIAvailable(app_id)) {
if (!file_handlers_enabled)
EnableAndRegisterOsFileHandlers(app_id);
} else if (file_handlers_enabled) {
DisableAndUnregisterOsFileHandlers(app_id, base::DoNothing());
}
}

void WebAppFileHandlerManager::DisableAutomaticFileHandlerCleanupForTesting() {
g_disable_automatic_file_handler_cleanup_for_testing = true;
}
Expand Down Expand Up @@ -338,12 +245,4 @@ const absl::optional<GURL> WebAppFileHandlerManager::GetMatchingFileHandlerURL(
return absl::nullopt;
}

bool WebAppFileHandlerManager::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
37 changes: 0 additions & 37 deletions chrome/browser/web_applications/web_app_file_handler_manager.h
View file
Expand Up @@ -18,10 +18,6 @@

class Profile;

namespace content {
class WebContents;
}

namespace web_app {

class WebAppRegistrar;
Expand Down Expand Up @@ -57,11 +53,6 @@ class WebAppFileHandlerManager {
// feature flag must also separately be enabled.
static void SetIconsSupportedByOsForTesting(bool value);

// Set a callback which is fired when the file handling expiry time is
// updated.
void SetOnFileHandlingExpiryUpdatedForTesting(
base::RepeatingCallback<void()> on_file_handling_expiry_updated);

// Returns |app_id|'s URL registered to handle |launch_files|'s extensions, or
// nullopt otherwise.
const absl::optional<GURL> GetMatchingFileHandlerURL(
Expand All @@ -79,24 +70,6 @@ class WebAppFileHandlerManager {
void DisableAndUnregisterOsFileHandlers(const AppId& app_id,
ResultCallback callback);

// Updates the file handling origin trial expiry timer based on a currently
// 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.
void ForceEnableFileHandlingOriginTrial(const AppId& app_id);

// Disable a force enabled File Handling origin trial. This will unregister
// 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.
Expand All @@ -121,23 +94,13 @@ class WebAppFileHandlerManager {
virtual const apps::FileHandlers* GetAllFileHandlers(const AppId& app_id);

private:
void OnOriginTrialExpiryTimeReceived(
mojo::AssociatedRemote<blink::mojom::FileHandlingExpiry> /*interface*/,
const AppId& app_id,
base::Time expiry_time);

// Removes file handlers whose origin trials have expired (assuming
// kFileHandlingAPI isn't enabled). Returns the number of apps that had file
// handlers unregistered, for use in tests.
int CleanupAfterOriginTrials();

void UpdateFileHandlersForOriginTrialExpiryTime(
const AppId& app_id,
const base::Time& expiry_time);

static bool disable_automatic_file_handler_cleanup_for_testing_;
bool disable_os_integration_for_testing_ = false;
base::RepeatingCallback<void()> on_file_handling_expiry_updated_for_testing_;

Profile* const profile_;
WebAppRegistrar* registrar_ = nullptr;
Expand Down
9 changes: 0 additions & 9 deletions chrome/browser/web_applications/web_app_tab_helper.cc
View file
Expand Up @@ -119,11 +119,6 @@ void WebAppTabHelper::DOMContentLoaded(
// app.
if (app_id_.empty())
return;

// There is no way to reliably know if |app_id_| is for a System Web App
// during startup, so we always call MaybeUpdateFileHandlingOriginTrialExpiry.
provider_->os_integration_manager().MaybeUpdateFileHandlingOriginTrialExpiry(
web_contents(), app_id_);
}

void WebAppTabHelper::DidCloneToNewWebContents(
Expand All @@ -149,10 +144,6 @@ void WebAppTabHelper::OnWebAppInstalled(const AppId& installed_app_id) {
return;

SetAppId(app_id);

// TODO(crbug.com/1053371): Clean up where we install file handlers.
provider_->os_integration_manager().MaybeUpdateFileHandlingOriginTrialExpiry(
web_contents(), installed_app_id);
}

void WebAppTabHelper::OnWebAppWillBeUninstalled(
Expand Down

0 comments on commit a9d44e5

Please sign in to comment.