Move console message output to HTMLIFrameElement::ConstructContainerPolicy

Previously there was a param |Vector<String>* messages| since the very
first version of HTMLFrameOwnerElement::ConstructContainerPolicy.
However, except the implementation in HTMLIFrameElement, other
implementations all ignore this param.

This CL moves output of console messages from caller of
ConstructContainerPolicy, i.e.
HTMLFrameOwnerElement::UpdateContainerPolicy, to
HTMLIFrameElement::ConstructContainerPolicy so that parameter
|messages| can be removed.

Change-Id: Ib2732474d881a13b8fe507af6ba63a526df3900d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209378
Commit-Queue: Charlie Hu <chenleihu@google.com>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772656}

third_party/blink/renderer/core/html/html_frame_element.cc

@@ -75,8 +75,7 @@ void HTMLFrameElement::ParseAttribute(
   }
 }
 
-ParsedFeaturePolicy HTMLFrameElement::ConstructContainerPolicy(
-    Vector<String>*) const {
+ParsedFeaturePolicy HTMLFrameElement::ConstructContainerPolicy() const {
   // Frame elements are not allowed to enable the fullscreen feature. Add an
   // empty allowlist for the fullscreen feature so that the framed content is
   // unable to use the API, regardless of origin.

third_party/blink/renderer/core/html/html_frame_element.h

@@ -41,8 +41,7 @@ class CORE_EXPORT HTMLFrameElement final : public HTMLFrameElementBase {
 
   bool NoResize() const;
 
-  ParsedFeaturePolicy ConstructContainerPolicy(
-      Vector<String>* /* messages */) const override;
+  ParsedFeaturePolicy ConstructContainerPolicy() const override;
 
   mojom::blink::FrameOwnerElementType OwnerType() const final {
     return mojom::blink::FrameOwnerElementType::kFrame;

third_party/blink/renderer/core/html/html_frame_owner_element.cc

@@ -279,7 +279,7 @@ void HTMLFrameOwnerElement::SetSandboxFlags(
   frame_policy_.sandbox_flags = flags;
   // Recalculate the container policy in case the allow-same-origin flag has
   // changed.
-  frame_policy_.container_policy = ConstructContainerPolicy(nullptr);
+  frame_policy_.container_policy = ConstructContainerPolicy();
 
   // Don't notify about updates if ContentFrame() is null, for example when
   // the subframe hasn't been created yet.
@@ -311,8 +311,8 @@ void HTMLFrameOwnerElement::DisposePluginSoon(WebPluginContainerImpl* plugin) {
     plugin->Dispose();
 }
 
-void HTMLFrameOwnerElement::UpdateContainerPolicy(Vector<String>* messages) {
-  frame_policy_.container_policy = ConstructContainerPolicy(messages);
+void HTMLFrameOwnerElement::UpdateContainerPolicy() {
+  frame_policy_.container_policy = ConstructContainerPolicy();
   // Don't notify about updates if ContentFrame() is null, for example when
   // the subframe hasn't been created yet.
   if (ContentFrame()) {

third_party/blink/renderer/core/html/html_frame_owner_element.h

@@ -168,12 +168,11 @@ class CORE_EXPORT HTMLFrameOwnerElement : public HTMLElement,
   // Return a feature policy container policy for this frame, based on the
   // frame attributes and the effective origin specified in the frame
   // attributes.
-  virtual ParsedFeaturePolicy ConstructContainerPolicy(
-      Vector<String>* /*  messages */) const = 0;
+  virtual ParsedFeaturePolicy ConstructContainerPolicy() const = 0;
 
   // Update the container policy and notify the frame loader client of any
   // changes.
-  void UpdateContainerPolicy(Vector<String>* messages = nullptr);
+  void UpdateContainerPolicy();
 
   // Return a document policy required policy for this frame, based on the
   // frame attributes.

third_party/blink/renderer/core/html/html_iframe_element.cc

@@ -196,17 +196,9 @@ void HTMLIFrameElement::ParseAttribute(
           current_flags & ~sandbox_to_set;
     }
     SetSandboxFlags(sandbox_to_set);
-    if (RuntimeEnabledFeatures::FeaturePolicyForSandboxEnabled()) {
-      Vector<String> messages;
-      UpdateContainerPolicy(&messages);
-      if (!messages.IsEmpty()) {
-        for (const String& message : messages) {
-          GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
-              mojom::ConsoleMessageSource::kOther,
-              mojom::ConsoleMessageLevel::kWarning, message));
-        }
-      }
-    }
+    if (RuntimeEnabledFeatures::FeaturePolicyForSandboxEnabled())
+      UpdateContainerPolicy();
+
     UseCounter::Count(GetDocument(), WebFeature::kSandboxViaIFrame);
   } else if (name == html_names::kReferrerpolicyAttr) {
     referrer_policy_ = network::mojom::ReferrerPolicy::kDefault;
@@ -256,15 +248,7 @@ void HTMLIFrameElement::ParseAttribute(
   } else if (name == html_names::kAllowAttr) {
     if (allow_ != value) {
       allow_ = value;
-      Vector<String> messages;
-      UpdateContainerPolicy(&messages);
-      if (!messages.IsEmpty()) {
-        for (const String& message : messages) {
-          GetDocument().AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
-              mojom::ConsoleMessageSource::kOther,
-              mojom::ConsoleMessageLevel::kWarning, message));
-        }
-      }
+      UpdateContainerPolicy();
       if (!value.IsEmpty()) {
         UseCounter::Count(GetDocument(),
                           WebFeature::kFeaturePolicyAllowAttribute);
@@ -348,29 +332,30 @@ DocumentPolicy::FeatureState HTMLIFrameElement::ConstructRequiredPolicy()
   return new_required_policy.feature_state;
 }
 
-ParsedFeaturePolicy HTMLIFrameElement::ConstructContainerPolicy(
-    Vector<String>* messages) const {
+ParsedFeaturePolicy HTMLIFrameElement::ConstructContainerPolicy() const {
   scoped_refptr<const SecurityOrigin> src_origin = GetOriginForFeaturePolicy();
   scoped_refptr<const SecurityOrigin> self_origin =
       GetDocument().GetSecurityOrigin();
 
+  Vector<String> messages;
+
   // Start with the allow attribute
   ParsedFeaturePolicy container_policy = FeaturePolicyParser::ParseAttribute(
-      allow_, self_origin, src_origin, messages, &GetDocument());
+      allow_, self_origin, src_origin, &messages, &GetDocument());
 
   // Next, process sandbox flags. These all only take effect if a corresponding
   // policy does *not* exist in the allow attribute's value.
   if (RuntimeEnabledFeatures::FeaturePolicyForSandboxEnabled()) {
     // If the frame is sandboxed at all, then warn if feature policy attributes
     // will override the sandbox attributes.
-    if (messages && (sandbox_flags_converted_to_feature_policies_ &
-                     network::mojom::blink::WebSandboxFlags::kNavigation) !=
-                        network::mojom::blink::WebSandboxFlags::kNone) {
+    if ((sandbox_flags_converted_to_feature_policies_ &
+         network::mojom::blink::WebSandboxFlags::kNavigation) !=
+        network::mojom::blink::WebSandboxFlags::kNone) {
       for (const auto& pair : SandboxFlagsWithFeaturePolicies()) {
         if ((sandbox_flags_converted_to_feature_policies_ & pair.first) !=
                 network::mojom::blink::WebSandboxFlags::kNone &&
             IsFeatureDeclared(pair.second, container_policy)) {
-          messages->push_back(String::Format(
+          messages.push_back(String::Format(
               "Allow and Sandbox attributes both mention '%s'. Allow will take "
               "precedence.",
               GetNameForFeature(pair.second).Utf8().c_str()));
@@ -390,8 +375,8 @@ ParsedFeaturePolicy HTMLIFrameElement::ConstructContainerPolicy(
   if (AllowFullscreen()) {
     bool policy_changed = AllowFeatureEverywhereIfNotPresent(
         mojom::blink::FeaturePolicyFeature::kFullscreen, container_policy);
-    if (!policy_changed && messages) {
-      messages->push_back(
+    if (!policy_changed) {
+      messages.push_back(
           "Allow attribute will take precedence over 'allowfullscreen'.");
     }
   }
@@ -400,8 +385,8 @@ ParsedFeaturePolicy HTMLIFrameElement::ConstructContainerPolicy(
   if (AllowPaymentRequest()) {
     bool policy_changed = AllowFeatureEverywhereIfNotPresent(
         mojom::blink::FeaturePolicyFeature::kPayment, container_policy);
-    if (!policy_changed && messages) {
-      messages->push_back(
+    if (!policy_changed) {
+      messages.push_back(
           "Allow attribute will take precedence over 'allowpaymentrequest'.");
     }
   }
@@ -411,6 +396,14 @@ ParsedFeaturePolicy HTMLIFrameElement::ConstructContainerPolicy(
   if (policy_)
     policy_->UpdateContainerPolicy(container_policy, src_origin);
 
+  for (const auto& message : messages) {
+    GetDocument().AddConsoleMessage(
+        MakeGarbageCollected<ConsoleMessage>(
+            mojom::blink::ConsoleMessageSource::kOther,
+            mojom::blink::ConsoleMessageLevel::kWarning, message),
+        /* discard_duplicates */ true);
+  }
+
   return container_policy;
 }
 

third_party/blink/renderer/core/html/html_iframe_element.h

@@ -55,8 +55,7 @@ class CORE_EXPORT HTMLIFrameElement final
   // Returns attributes that should be checked against Trusted Types
   const AttrNameToTrustedType& GetCheckedAttributeTypes() const override;
 
-  ParsedFeaturePolicy ConstructContainerPolicy(
-      Vector<String>* /* messages */) const override;
+  ParsedFeaturePolicy ConstructContainerPolicy() const override;
   DocumentPolicy::FeatureState ConstructRequiredPolicy() const override;
 
   mojom::blink::FrameOwnerElementType OwnerType() const final {

third_party/blink/renderer/core/html/html_iframe_element_test.cc

@@ -276,7 +276,7 @@ TEST_F(HTMLIFrameElementTest, SameOriginSandboxAttributeContainerPolicy) {
 // iframe element.
 TEST_F(HTMLIFrameElementTest, ConstructEmptyContainerPolicy) {
   ParsedFeaturePolicy container_policy =
-      frame_element_->ConstructContainerPolicy(nullptr);
+      frame_element_->ConstructContainerPolicy();
   EXPECT_EQ(0UL, container_policy.size());
 }
 
@@ -285,7 +285,7 @@ TEST_F(HTMLIFrameElementTest, ConstructEmptyContainerPolicy) {
 TEST_F(HTMLIFrameElementTest, ConstructContainerPolicy) {
   frame_element_->setAttribute(html_names::kAllowAttr, "payment; usb");
   ParsedFeaturePolicy container_policy =
-      frame_element_->ConstructContainerPolicy(nullptr);
+      frame_element_->ConstructContainerPolicy();
   EXPECT_EQ(2UL, container_policy.size());
   EXPECT_EQ(mojom::blink::FeaturePolicyFeature::kPayment,
             container_policy[0].feature);
@@ -306,7 +306,7 @@ TEST_F(HTMLIFrameElementTest, ConstructContainerPolicyWithAllowFullscreen) {
   frame_element_->SetBooleanAttribute(html_names::kAllowfullscreenAttr, true);
 
   ParsedFeaturePolicy container_policy =
-      frame_element_->ConstructContainerPolicy(nullptr);
+      frame_element_->ConstructContainerPolicy();
   EXPECT_EQ(1UL, container_policy.size());
   EXPECT_EQ(mojom::blink::FeaturePolicyFeature::kFullscreen,
             container_policy[0].feature);
@@ -321,7 +321,7 @@ TEST_F(HTMLIFrameElementTest, ConstructContainerPolicyWithAllowPaymentRequest) {
                                       true);
 
   ParsedFeaturePolicy container_policy =
-      frame_element_->ConstructContainerPolicy(nullptr);
+      frame_element_->ConstructContainerPolicy();
   EXPECT_EQ(2UL, container_policy.size());
   EXPECT_EQ(mojom::blink::FeaturePolicyFeature::kUsb,
             container_policy[0].feature);
@@ -346,7 +346,7 @@ TEST_F(HTMLIFrameElementTest, ConstructContainerPolicyWithAllowAttributes) {
                                       true);
 
   ParsedFeaturePolicy container_policy =
-      frame_element_->ConstructContainerPolicy(nullptr);
+      frame_element_->ConstructContainerPolicy();
   EXPECT_EQ(3UL, container_policy.size());
   EXPECT_EQ(mojom::blink::FeaturePolicyFeature::kPayment,
             container_policy[0].feature);
@@ -385,4 +385,25 @@ TEST_F(HTMLIFrameElementSimTest, PolicyAttributeParsingError) {
   }
 }
 
+TEST_F(HTMLIFrameElementSimTest, AllowAttributeParsingError) {
+  SimRequest main_resource("https://example.com", "text/html");
+  LoadURL("https://example.com");
+  main_resource.Complete(R"(
+    <iframe
+      allow="bad-feature-name"
+      allowfullscreen
+      allowpayment
+      sandbox=""></iframe>
+  )");
+
+  EXPECT_EQ(ConsoleMessages().size(), 1u)
+      << "Allow attribute parsing should only generate console message once, "
+         "even though there might be multiple call to "
+         "FeaturePolicyParser::ParseAttribute.";
+  EXPECT_TRUE(ConsoleMessages().front().StartsWith("Unrecognized feature"))
+      << "Expect feature policy parser raising error for unrecognized feature "
+         "but got: "
+      << ConsoleMessages().front();
+}
+
 }  // namespace blink

third_party/blink/renderer/core/html/html_plugin_element.cc

@@ -272,8 +272,7 @@ bool HTMLPlugInElement::ShouldAccelerate() const {
   return plugin && plugin->CcLayer();
 }
 
-ParsedFeaturePolicy HTMLPlugInElement::ConstructContainerPolicy(
-    Vector<String>*) const {
+ParsedFeaturePolicy HTMLPlugInElement::ConstructContainerPolicy() const {
   // Plugin elements (<object> and <embed>) are not allowed to enable the
   // fullscreen feature. Add an empty allowlist for the fullscreen feature so
   // that the nested browsing context is unable to use the API, regardless of

third_party/blink/renderer/core/html/html_plugin_element.h

@@ -99,8 +99,7 @@ class CORE_EXPORT HTMLPlugInElement
 
   bool ShouldAccelerate() const;
 
-  ParsedFeaturePolicy ConstructContainerPolicy(
-      Vector<String>* /* messages */) const override;
+  ParsedFeaturePolicy ConstructContainerPolicy() const override;
 
   bool IsImageType() const;
   HTMLImageLoader* ImageLoader() const { return image_loader_.Get(); }

third_party/blink/renderer/core/html/portal/html_portal_element.h

@@ -120,7 +120,7 @@ class CORE_EXPORT HTMLPortalElement : public HTMLFrameOwnerElement {
 
   // HTMLFrameOwnerElement overrides
   void DisconnectContentFrame() override;
-  ParsedFeaturePolicy ConstructContainerPolicy(Vector<String>*) const override {
+  ParsedFeaturePolicy ConstructContainerPolicy() const override {
     return ParsedFeaturePolicy();
   }
   void AttachLayoutTree(AttachContext& context) override;