Fix the AllowPopupsDuringPageUnload policy behaviour.

- Check for the policy's pref value before appending its switch

Bug: 949184
Change-Id: Ib25e61ada0ba0592e6b91619313ec402c5a8a062
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1578605
Commit-Queue: Amr Aboelkher <amraboelkher@google.com>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654852}

chrome/browser/chrome_content_browser_client.cc

@@ -2110,7 +2110,6 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
     MaybeCopyDisableWebRtcEncryptionSwitch(command_line,
                                            browser_command_line,
                                            chrome::GetChannel());
-
     if (process) {
       PrefService* prefs = profile->GetPrefs();
       // Currently this pref is only registered if applied via a policy.
@@ -2162,9 +2161,10 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
             network::switches::kUnsafelyTreatInsecureOriginAsSecure,
             prefs->GetString(prefs::kUnsafelyTreatInsecureOriginAsSecure));
       }
-
-      if (prefs->HasPrefPath(prefs::kAllowPopupsDuringPageUnload))
+      if (prefs->HasPrefPath(prefs::kAllowPopupsDuringPageUnload) &&
+          prefs->GetBoolean(prefs::kAllowPopupsDuringPageUnload)) {
         command_line->AppendSwitch(switches::kAllowPopupsDuringPageUnload);
+      }
     }
 
     if (IsAutoReloadEnabled())

chrome/browser/chrome_content_browser_client_browsertest.cc

@@ -24,12 +24,14 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/network_session_configurator/common/network_switches.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/policy_header_service.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_frame_host.h"
@@ -98,6 +100,55 @@ IN_PROC_BROWSER_TEST_F(ChromeContentBrowserClientBrowserTest,
   EXPECT_EQ(url, entry->GetVirtualURL());
 }
 
+class ChromeContentBrowserClientPopupsTest : public InProcessBrowserTest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // Required setup for kAllowPopupsDuringPageUnload switch
+    // as its being checked (whether its going to be enabled or not)
+    // only if the process type is renderer process.
+    command_line_.AppendSwitchASCII(switches::kProcessType,
+                                    switches::kRendererProcess);
+  }
+  void SetUpOnMainThread() override {
+    kChildProcessId = browser()
+                          ->tab_strip_model()
+                          ->GetActiveWebContents()
+                          ->GetMainFrame()
+                          ->GetProcess()
+                          ->GetID();
+  }
+  ChromeContentBrowserClientPopupsTest()
+      : command_line_(base::CommandLine::NO_PROGRAM) {}
+
+  void AppendContentBrowserClientSwitches() {
+    client_.AppendExtraCommandLineSwitches(&command_line_, kChildProcessId);
+  }
+
+  const base::CommandLine& command_line() const { return command_line_; }
+
+ private:
+  ChromeContentBrowserClient client_;
+  base::CommandLine command_line_;
+  int kChildProcessId;
+};
+
+IN_PROC_BROWSER_TEST_F(ChromeContentBrowserClientPopupsTest,
+                       AllowPopupsDuringPageUnload) {
+  // Verify that the switch is included only when the
+  // pref AllowPopupsDuringPageUnload value is true.
+
+  PrefService* pref_service = browser()->profile()->GetPrefs();
+  pref_service->SetBoolean(prefs::kAllowPopupsDuringPageUnload, false);
+  AppendContentBrowserClientSwitches();
+  EXPECT_FALSE(
+      command_line().HasSwitch(switches::kAllowPopupsDuringPageUnload));
+  // When the pref value is being set to true
+  // the switch should be included.
+  pref_service->SetBoolean(prefs::kAllowPopupsDuringPageUnload, true);
+  AppendContentBrowserClientSwitches();
+  EXPECT_TRUE(command_line().HasSwitch(switches::kAllowPopupsDuringPageUnload));
+}
+
 // Helper class to mark "https://ntp.com/" as an isolated origin.
 class IsolatedOriginNTPBrowserTest : public InProcessBrowserTest,
                                      public InstantTestBase {