Bug 980243. Make sure that image src sets do something even if the src is being set to the value it already has. r=sicking

content/html/content/src/HTMLImageElement.cpp

@@ -322,10 +322,9 @@ HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
     CancelImageRequests(aNotify);
   }
 
-  // If we plan to call LoadImage, we want to do it first so that the image load
-  // kicks off. But if aNotify is false, we are coming from the 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. Skip the LoadImage call in that case.
+  // If aNotify is false, we are coming from the 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. Skip the LoadImage call in that case.
   if (aNotify &&
       aNameSpaceID == kNameSpaceID_None &&
       aName == nsGkAtoms::crossorigin) {
@@ -336,29 +335,6 @@ HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
     LoadImage(uri, true, aNotify);
   }
 
-  if (aNotify &&
-      aNameSpaceID == kNameSpaceID_None &&
-      aName == nsGkAtoms::src &&
-      aValue) {
-
-    // Prevent setting image.src by exiting early
-    if (nsContentUtils::IsImageSrcSetDisabled()) {
-      return NS_OK;
-    }
-
-    // A hack to get animations to reset. See bug 594771.
-    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
-    // the state gets in Element's attr-setting happen around this
-    // LoadImage call, we could start passing false instead of aNotify
-    // here.
-    LoadImage(aValue->GetStringValue(), true, aNotify);
-
-    mNewRequestsWillNeedAnimationReset = false;
-  }
-
   return nsGenericHTMLElement::AfterSetAttr(aNameSpaceID, aName,
                                             aValue, aNotify);
 }
@@ -426,17 +402,40 @@ HTMLImageElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                           nsIAtom* aPrefix, const nsAString& aValue,
                           bool aNotify)
 {
+  // 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
+  // no-op.
+  //
+  // If aNotify is false, we are coming from the 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. Skip the LoadImage call in that case.
+  if (aNotify &&
+      aNameSpaceID == kNameSpaceID_None &&
+      aName == nsGkAtoms::src) {
+
+    // Prevent setting image.src by exiting early
+    if (nsContentUtils::IsImageSrcSetDisabled()) {
+      return NS_OK;
+    }
+
+    // A hack to get animations to reset. See bug 594771.
+    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
+    // the state gets in Element's attr-setting happen around this
+    // LoadImage call, we could start passing false instead of aNotify
+    // here.
+    LoadImage(aValue, true, aNotify);
+
+    mNewRequestsWillNeedAnimationReset = false;
+  }
+
   return nsGenericHTMLElement::SetAttr(aNameSpaceID, aName, aPrefix, aValue,
                                        aNotify);
 }
 
-nsresult
-HTMLImageElement::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                            bool aNotify)
-{
-  return nsGenericHTMLElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
-}
-
 nsresult
 HTMLImageElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                              nsIContent* aBindingParent,

content/html/content/src/HTMLImageElement.h

@@ -65,8 +65,6 @@ public:
   virtual nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
                            nsIAtom* aPrefix, const nsAString& aValue,
                            bool aNotify) MOZ_OVERRIDE;
-  virtual nsresult UnsetAttr(int32_t aNameSpaceID, nsIAtom* aAttribute,
-                             bool aNotify) MOZ_OVERRIDE;
 
   virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                               nsIContent* aBindingParent,

content/html/content/test/mochitest.ini

@@ -435,6 +435,7 @@ skip-if = buildapp == 'b2g' # b2g(multiple concurrent window.open()s fail on B2G
 [test_iframe_sandbox_same_origin.html]
 [test_iframe_sandbox_workers.html]
 [test_img_attributes_reflection.html]
+[test_imageSrcSet.html]
 [test_li_attributes_reflection.html]
 [test_link_attributes_reflection.html]
 [test_map_attributes_reflection.html]

content/html/content/test/test_imageSrcSet.html

@@ -0,0 +1,38 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=980243
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 980243</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+  /** Test for Bug 980243 **/
+  SimpleTest.waitForExplicitFinish();
+
+  addLoadEvent(function() {
+    var img = document.querySelector("img");
+    img.onload = function() {
+      ok(true, "Reached here");
+      SimpleTest.finish();
+    }
+    // If ths spec ever changes to treat .src sets differently from
+    // setAttribute("src"), we'll need some sort of canonicalization step
+    // earlier to make the attr value an absolute URI.
+    img.setAttribute("src", img.getAttribute("src"));
+  });
+  </script>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=980243">Mozilla Bug 980243</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <img src="file_formSubmission_img.jpg">
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>