Issue #2736 - Part 4: Re-work img <src> attribute.

Use subject principal as triggering principal in <img> "src" attribute.
Also get rid of the `BeforeMaybeChangeAttr`/`AfterMaybeChangeAttr` dance:
It makes more logical sense for these effects to happen _after_ the
attribute has actually been changed.

dom/base/Element.cpp

@@ -4053,9 +4053,16 @@ Element::GetReferrerPolicyAsEnum()
   if (Preferences::GetBool("network.http.enablePerElementReferrer", true) &&
       IsHTMLElement()) {
     const nsAttrValue* referrerValue = GetParsedAttr(nsGkAtoms::referrerpolicy);
-    if (referrerValue && referrerValue->Type() == nsAttrValue::eEnum) {
+    return ReferrerPolicyFromAttr(referrerValue);
-      return net::ReferrerPolicy(referrerValue->GetEnumValue());
   }
+  return net::RP_Unset;
+}
+
+net::ReferrerPolicy
+Element::ReferrerPolicyFromAttr(const nsAttrValue* aValue)
+{
+  if (aValue && aValue->Type() == nsAttrValue::eEnum) {
+    return net::ReferrerPolicy(aValue->GetEnumValue());
   }
   return net::RP_Unset;
 }

dom/base/Element.h

@@ -1265,6 +1265,7 @@ public:
   float FontSizeInflation();
 
   net::ReferrerPolicy GetReferrerPolicyAsEnum();
+  net::ReferrerPolicy ReferrerPolicyFromAttr(const nsAttrValue* aValue);
 
   /*
    * Helpers for .dataset.  This is implemented on Element, though only some

dom/base/nsContentUtils.cpp

@@ -199,6 +199,7 @@
 #include "nsThreadUtils.h"
 #include "nsUnicharUtilCIID.h"
 #include "nsUnicodeProperties.h"
+#include "nsURLHelper.h"
 #include "nsViewManager.h"
 #include "nsViewportInfo.h"
 #include "nsWidgetsCID.h"
@@ -2118,6 +2119,54 @@ nsContentUtils::CanCallerAccess(nsPIDOMWindowInner* aWindow)
   return CanCallerAccess(SubjectPrincipal(), scriptObject->GetPrincipal());
 }
 
+// static
+nsIPrincipal*
+nsContentUtils::GetAttrTriggeringPrincipal(nsIContent* aContent, const nsAString& aAttrValue,
+                                           nsIPrincipal* aSubjectPrincipal)
+{
+  nsIPrincipal* contentPrin = aContent ? aContent->NodePrincipal() : nullptr;
+
+  // If the subject principal is the same as the content principal, or no
+  // explicit subject principal was provided, we don't need to do any further
+  // checks. Just return the content principal.
+  if (contentPrin == aSubjectPrincipal || !aSubjectPrincipal) {
+    return contentPrin;
+  }
+
+  // If the attribute value is empty, it's not an absolute URL, so don't bother
+  // with more expensive checks.
+  if (!aAttrValue.IsEmpty() &&
+      IsAbsoluteURL(NS_ConvertUTF16toUTF8(aAttrValue))) {
+    return aSubjectPrincipal;
+  }
+
+  return contentPrin;
+}
+
+// static
+bool
+nsContentUtils::IsAbsoluteURL(const nsACString& aURL)
+{
+  nsAutoCString scheme;
+  if (NS_FAILED(net_ExtractURLScheme(aURL, scheme))) {
+    // If we can't extract a scheme, it's not an absolute URL.
+    return false;
+  }
+
+  // If it parses as an absolute StandardURL, it's definitely an absolute URL,
+  // so no need to check with the IO service.
+  if (net_IsAbsoluteURL(aURL)) {
+    return true;
+  }
+
+  uint32_t flags;
+  if (NS_SUCCEEDED(sIOService->GetProtocolFlags(scheme.get(), &flags))) {
+    return flags & nsIProtocolHandler::URI_NORELATIVE;
+  }
+
+  return false;
+}
+
 //static
 bool
 nsContentUtils::InProlog(nsINode *aNode)

dom/base/nsContentUtils.h

@@ -526,6 +526,40 @@ public:
   // aWindow can be either outer or inner window.
   static bool CanCallerAccess(nsPIDOMWindowInner* aWindow);
 
+  /**
+   * Returns the triggering principal which should be used for the given URL
+   * attribute value with the given subject principal.
+   *
+   * If the attribute value is not an absolute URL, the subject principal will
+   * be ignored, and the node principal of aContent will be used instead.
+   * If aContent is non-null, this function will always return a principal.
+   * Otherewise, it may return null if aSubjectPrincipal is null or is rejected
+   * based on the attribute value.
+   *
+   * @param aContent The content on which the attribute is being set.
+   * @param aAttrValue The URL value of the attribute. For parsed attribute
+   *        values, such as `srcset`, this function should be called separately
+   *        for each URL value it contains.
+   * @param aSubjectPrincipal The subject principal of the scripted caller
+   *        responsible for setting the attribute, or null if no scripted caller
+   *        can be determined.
+   */
+  static nsIPrincipal* GetAttrTriggeringPrincipal(nsIContent* aContent,
+                                                  const nsAString& aAttrValue,
+                                                  nsIPrincipal* aSubjectPrincipal);
+
+  /**
+   * Returns true if the given string is guaranteed to be treated as an absolute
+   * URL, rather than a relative URL. In practice, this means any complete URL
+   * as supported by nsStandardURL, or any string beginning with a valid scheme
+   * which is known to the IO service, and has the URI_NORELATIVE flag.
+   *
+   * If the URL may be treated as absolute in some cases, but relative in others
+   * (for instance, "http:foo", which can be either an absolute or relative URL,
+   * depending on the context), this function returns false.
+   */
+  static bool IsAbsoluteURL(const nsACString& aURL);
+
   /**
    * GetDocumentFromCaller gets its document by looking at the last called
    * function and finding the document that the function itself relates to.

dom/base/nsImageLoadingContent.cpp

@@ -734,7 +734,8 @@ nsresult
 nsImageLoadingContent::LoadImage(const nsAString& aNewURI,
                                  bool aForce,
                                  bool aNotify,
-                                 ImageLoadType aImageLoadType)
+                                 ImageLoadType aImageLoadType,
+                                 nsIPrincipal* aTriggeringPrincipal)
 {
   // First, get a document (needed for security checks and the like)
   nsIDocument* doc = GetOurOwnerDoc();
@@ -768,7 +769,8 @@ nsImageLoadingContent::LoadImage(const nsAString& aNewURI,
 
   NS_TryToSetImmutable(imageURI);
 
-  return LoadImage(imageURI, aForce, aNotify, aImageLoadType, false, doc);
+  return LoadImage(imageURI, aForce, aNotify, aImageLoadType, false, doc,
+                   nsIRequest::LOAD_NORMAL, aTriggeringPrincipal);
 }
 
 nsresult
@@ -778,7 +780,8 @@ nsImageLoadingContent::LoadImage(nsIURI* aNewURI,
                                  ImageLoadType aImageLoadType,
                                  bool aLoadStart,
                                  nsIDocument* aDocument,
-                                 nsLoadFlags aLoadFlags)
+                                 nsLoadFlags aLoadFlags,
+                                 nsIPrincipal* aTriggeringPrincipal)
 {
   MOZ_ASSERT(!mIsStartingImageLoad, "some evil code is reentering LoadImage.");
   if (mIsStartingImageLoad) {
@@ -886,7 +889,7 @@ nsImageLoadingContent::LoadImage(nsIURI* aNewURI,
   nsresult rv = nsContentUtils::LoadImage(aNewURI,
                                           thisNode,
                                           aDocument,
-                                          aDocument->NodePrincipal(),
+                                          aTriggeringPrincipal ? aTriggeringPrincipal : aDocument->NodePrincipal(),
                                           aDocument->GetDocumentURI(),
                                           referrerPolicy,
                                           this, loadFlags,

dom/base/nsImageLoadingContent.h

@@ -102,9 +102,12 @@ protected:
    * @param aNotify If true, nsIDocumentObserver state change notifications
    *                will be sent as needed.
    * @param aImageLoadType The ImageLoadType for this request
+   * @param aTriggeringPrincipal Optional parameter specifying the triggering
+   *        principal to use for the image load
    */
   nsresult LoadImage(const nsAString& aNewURI, bool aForce,
-                     bool aNotify, ImageLoadType aImageLoadType);
+                     bool aNotify, ImageLoadType aImageLoadType,
+                     nsIPrincipal* aTriggeringPrincipal = nullptr);
 
   /**
    * ImageState is called by subclasses that are computing their content state.
@@ -134,11 +137,23 @@ protected:
    *        This is purely a performance optimization.
    * @param aLoadFlags Optional parameter specifying load flags to use for
    *        the image load
+   * @param aTriggeringPrincipal Optional parameter specifying the triggering
+   *        principal to use for the image load
    */
   nsresult LoadImage(nsIURI* aNewURI, bool aForce, bool aNotify,
                      ImageLoadType aImageLoadType, bool aLoadStart = true,
                      nsIDocument* aDocument = nullptr,
-                     nsLoadFlags aLoadFlags = nsIRequest::LOAD_NORMAL);
+                     nsLoadFlags aLoadFlags = nsIRequest::LOAD_NORMAL,
+                     nsIPrincipal* aTriggeringPrincipal = nullptr);
+  // Simplified version to pass triggering principal with defaults otherwise.
+  nsresult LoadImage(nsIURI* aNewURI, bool aForce, bool aNotify,
+                     ImageLoadType aImageLoadType,
+                     nsIPrincipal* aTriggeringPrincipal)
+  {
+    return LoadImage(aNewURI, aForce, aNotify, aImageLoadType,
+                     true, nullptr, nsIRequest::LOAD_NORMAL,
+                     aTriggeringPrincipal);
+  }
 
   /**
    * helpers to get the document for this content (from the nodeinfo

dom/html/HTMLImageElement.cpp

@@ -109,7 +109,6 @@ private:
 HTMLImageElement::HTMLImageElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo)
   : nsGenericHTMLElement(aNodeInfo)
   , mForm(nullptr)
-  , mForceReload(false)
   , mInDocResponsiveContent(false)
   , mCurrentDensity(1.0)
 {
@@ -370,10 +369,6 @@ HTMLImageElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                 const nsAttrValueOrString* aValue,
                                 bool aNotify)
 {
-  if (aValue) {
-    BeforeMaybeChangeAttr(aNameSpaceID, aName, *aValue, aNotify);
-  }
-
   if (aNameSpaceID == kNameSpaceID_None && mForm &&
       (aName == nsGkAtoms::name || aName == nsGkAtoms::id)) {
     // remove the image from the hashtable as needed
@@ -397,8 +392,11 @@ HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                                nsIPrincipal* aMaybeScriptedPrincipal,
                                bool aNotify)
 {
+  nsAttrValueOrString attrVal(aValue);
+  
   if (aValue) {
-    AfterMaybeChangeAttr(aNameSpaceID, aName, aMaybeScriptedPrincipal, aNotify);
+    AfterMaybeChangeAttr(aNameSpaceID, aName, attrVal, aOldValue, true,
+                         aMaybeScriptedPrincipal, aNotify);
   }
 
   if (aNameSpaceID == kNameSpaceID_None && mForm &&
@@ -415,8 +413,6 @@ HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
   // parser or some such place; we'll get bound after all the attributes have
   // been set, so we'll do the image load from BindToTree.
 
-  nsAttrValueOrString attrVal(aValue);
-
   if (aName == nsGkAtoms::src &&
       aNameSpaceID == kNameSpaceID_None &&
       !aValue) {
@@ -451,18 +447,21 @@ HTMLImageElement::OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName,
                                          const nsAttrValueOrString& aValue,
                                          bool aNotify)
 {
-  BeforeMaybeChangeAttr(aNamespaceID, aName, aValue, aNotify);
+  AfterMaybeChangeAttr(aNamespaceID, aName, aValue, nullptr, false, nullptr, aNotify);
-  AfterMaybeChangeAttr(aNamespaceID, aName, nullptr, aNotify);
 
   return nsGenericHTMLElement::OnAttrSetButNotChanged(aNamespaceID, aName,
                                                       aValue, aNotify);
 }
 
 void
-HTMLImageElement::BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+HTMLImageElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
                                        const nsAttrValueOrString& aValue,
+                                       const nsAttrValue* aOldValue,
+                                       bool aValueMaybeChanged,
+                                       nsIPrincipal* aMaybeScriptedPrincipal,
                                        bool aNotify)
 {
+  bool forceReload = false;
   // We need to force our image to reload.  This must be done here, not in
   // AfterSetAttr or BeforeSetAttr, because we want to do it even if the attr is
   // being set to its existing value, which is normally optimized away as a
@@ -473,12 +472,12 @@ HTMLImageElement::BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
   // spec.
   //
   // Both cases handle unsetting src in AfterSetAttr
-  //
-  // Much of this should probably happen in AfterMaybeChangeAttr.
-  // See Bug 1370705
   if (aNamespaceID == kNameSpaceID_None &&
       aName == nsGkAtoms::src) {
 
+    mSrcTriggeringPrincipal = 
+      nsContentUtils::GetAttrTriggeringPrincipal(this, aValue.String(), aMaybeScriptedPrincipal);
+
     if (InResponsiveMode()) {
       if (mResponsiveSelector &&
           mResponsiveSelector->Content() == this) {
@@ -496,53 +495,46 @@ HTMLImageElement::BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
       mNewRequestsWillNeedAnimationReset = true;
 
       // Force image loading here, so that we'll try to load the image from
-      // network if it's set to be not cacheable...  If we change things so that
+      // network if it's set to be not cacheable.
-      // the state gets in Element's attr-setting happen around this
+      // Potentially, false could be passed here rather than aNotify since
-      // LoadImage call, we could start passing false instead of aNotify
+      // UpdateState will be called by SetAttrAndNotify, but there are two
-      // here.
+      // obstacles to this: 1) LoadImage will end up calling
-      LoadImage(aValue.String(), true, aNotify, eImageLoadType_Normal);
+      // UpdateState(aNotify), and we do not want it to call UpdateState(false)
+      // when aNotify is true, and 2) When this function is called by
+      // OnAttrSetButNotChanged, SetAttrAndNotify will not subsequently call
+      // UpdateState.
+      LoadImage(aValue.String(), true, aNotify, eImageLoadType_Normal, mSrcTriggeringPrincipal);
 
       mNewRequestsWillNeedAnimationReset = false;
     }
   } else if (aNamespaceID == kNameSpaceID_None &&
              aName == nsGkAtoms::crossorigin &&
              aNotify) {
-    nsAttrValue attrValue;
+    if (aValueMaybeChanged && GetCORSMode() != AttrValueToCORSMode(aOldValue)) {
-    ParseCORSValue(aValue.String(), attrValue);
-    if (GetCORSMode() != AttrValueToCORSMode(&attrValue)) {
       // Force a new load of the image with the new cross origin policy.
-      mForceReload = true;
+      forceReload = true;
     }
   } else if (aName == nsGkAtoms::referrerpolicy &&
       aNamespaceID == kNameSpaceID_None &&
       aNotify) {
-    ReferrerPolicy referrerPolicy = AttributeReferrerPolicyFromString(aValue.String());
+    ReferrerPolicy referrerPolicy = GetImageReferrerPolicy();
     if (!InResponsiveMode() &&
         referrerPolicy != RP_Unset &&
-        referrerPolicy != GetImageReferrerPolicy()) {
+        aValueMaybeChanged &&
+        referrerPolicy != ReferrerPolicyFromAttr(aOldValue)) {
       // XXX: Bug 1076583 - We still use the older synchronous algorithm
       // Because referrerPolicy is not treated as relevant mutations, setting
       // the attribute will neither trigger a reload nor update the referrer
       // policy of the loading channel (whether it has previously completed or
       // not). Force a new load of the image with the new referrerpolicy.
-      mForceReload = true;
+      forceReload = true;
     }
   }
 
-  return;
-}
-
-void
-HTMLImageElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
-                                       nsIPrincipal* aMaybeScriptedPrincipal,
-                                       bool aNotify)
-{
   // Because we load image synchronously in non-responsive-mode, we need to do
   // reload after the attribute has been set if the reload is triggerred by
   // cross origin changing.
-  if (mForceReload) {
+  if (forceReload) {
-    mForceReload = false;
-
     if (InResponsiveMode()) {
       // per spec, full selection runs when this changes, even though
       // it doesn't directly affect the source selection
@@ -554,8 +546,6 @@ HTMLImageElement::AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
       ForceReload(aNotify);
     }
   }
-
-  return;
 }
 
 nsresult
@@ -1037,7 +1027,8 @@ HTMLImageElement::LoadSelectedImage(bool aForce, bool aNotify, bool aAlwaysLoad)
       // valid responsive sources from either, per spec.
       rv = LoadImage(src, aForce, aNotify,
                      HaveSrcsetOrInPicture() ? eImageLoadType_Imageset
-                                             : eImageLoadType_Normal);
+                                             : eImageLoadType_Normal,
+                     mSrcTriggeringPrincipal);
     }
   }
   mLastSelectedSource = selectedSource;

dom/html/HTMLImageElement.h

@@ -137,9 +137,13 @@ public:
   {
     SetHTMLAttr(nsGkAtoms::alt, aAlt, aError);
   }
-  void SetSrc(const nsAString& aSrc, ErrorResult& aError)
+  void GetSrc(nsAString& aSrc, nsIPrincipal&)
   {
-    SetHTMLAttr(nsGkAtoms::src, aSrc, aError);
+    GetURIAttr(nsGkAtoms::src, nullptr, aSrc);
+  }
+  void SetSrc(const nsAString& aSrc, nsIPrincipal& aTriggeringPrincipal, ErrorResult& aError)
+  {
+    SetHTMLAttr(nsGkAtoms::src, aSrc, aTriggeringPrincipal, aError);
   }
   void SetSrcset(const nsAString& aSrcset, ErrorResult& aError)
   {
@@ -365,31 +369,24 @@ private:
    * @param aName the localname of the attribute being set
    * @param aValue the value it's being set to represented as either a string or
    *        a parsed nsAttrValue.
-   * @param aNotify Whether we plan to notify document observers.
+   * @param aOldValue the value previously set. Will be null if no value was
-   */
+   *        previously set. This value should only be used when
-  void BeforeMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+   *        aValueMaybeChanged is true; when aValueMaybeChanged is false,
-                             const nsAttrValueOrString& aValue,
+   *        aOldValue should be considered unreliable.
-                             bool aNotify);
+   * @param aValueMaybeChanged will be false when this function is called from
-  /**
+   *        OnAttrSetButNotChanged to indicate that the value was not changed.
-   * This function is called by AfterSetAttr and OnAttrSetButNotChanged.
-   * It will not be called if the value is being unset.
-   *
-   * @param aNamespaceID the namespace of the attr being set
-   * @param aName the localname of the attribute being set
    * @param aNotify Whether we plan to notify document observers.
    */
   void AfterMaybeChangeAttr(int32_t aNamespaceID, nsIAtom* aName,
+                            const nsAttrValueOrString& aValue,
+                            const nsAttrValue* aOldValue,
+                            bool aValueMaybeChanged,
                             nsIPrincipal* aMaybeScriptedPrincipal,
                             bool aNotify);
-  /**
-   * Used by BeforeMaybeChangeAttr and AfterMaybeChangeAttr to keep track of
-   * whether a reload needs to be forced after an attribute change that is
-   * currently in progress.
-   */
-  bool mForceReload;
 
   bool mInDocResponsiveContent;
   RefPtr<ImageLoadTask> mPendingImageLoadTask;
+  nsCOMPtr<nsIPrincipal> mSrcTriggeringPrincipal;
 
   // Last URL that was attempted to load by this element.
   nsCOMPtr<nsIURI> mLastSelectedSource;

dom/webidl/HTMLImageElement.webidl

@@ -21,7 +21,7 @@ interface nsIStreamListener;
 interface HTMLImageElement : HTMLElement {
            [CEReactions, SetterThrows]
            attribute DOMString alt;
-           [CEReactions, SetterThrows]
+           [CEReactions, NeedsSubjectPrincipal, SetterThrows]
            attribute DOMString src;
            [CEReactions, SetterThrows]
            attribute DOMString srcset;