Bug 485553. Fix issue with mutating anchor uri properties and visited state, and cache the resulting URI when the URI is mutated instead of just reparsing it later. r+sr=jst

content/html/content/src/nsGenericHTMLElement.cpp

@@ -1060,11 +1060,17 @@ nsGenericHTMLElement::GetHrefURIForAnchors(nsIURI** aURI) const
   // Get href= attribute (relative URI).
 
   // We use the nsAttrValue's copy of the URI string to avoid copying.
-  GetURIAttr(nsGkAtoms::href, nsnull, aURI);
+  GetURIAttr(nsGkAtoms::href, nsnull, PR_FALSE, aURI);
 
   return NS_OK;
 }
 
+void
+nsGenericHTMLElement::GetHrefURIToMutate(nsIURI** aURI)
+{
+  GetURIAttr(nsGkAtoms::href, nsnull, PR_TRUE, aURI);
+}
+
 nsresult
 nsGenericHTMLElement::AfterSetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
                                    const nsAString* aValue, PRBool aNotify)
@@ -2140,7 +2146,7 @@ nsresult
 nsGenericHTMLElement::GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr, nsAString& aResult)
 {
   nsCOMPtr<nsIURI> uri;
-  PRBool hadAttr = GetURIAttr(aAttr, aBaseAttr, getter_AddRefs(uri));
+  PRBool hadAttr = GetURIAttr(aAttr, aBaseAttr, PR_FALSE, getter_AddRefs(uri));
   if (!hadAttr) {
     aResult.Truncate();
     return NS_OK;
@@ -2160,7 +2166,7 @@ nsGenericHTMLElement::GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr, nsAString&
 
 PRBool
 nsGenericHTMLElement::GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr,
-                                 nsIURI** aURI) const
+                                 PRBool aCloneIfCached, nsIURI** aURI) const
 {
   *aURI = nsnull;
 
@@ -2172,7 +2178,13 @@ nsGenericHTMLElement::GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr,
   PRBool isURIAttr = (attr->Type() == nsAttrValue::eLazyURIValue);
 
   if (isURIAttr && (*aURI = attr->GetURIValue())) {
-    NS_ADDREF(*aURI);
+    if (aCloneIfCached) {
+      nsIURI* clone = nsnull;
+      (*aURI)->Clone(&clone);
+      *aURI = clone;
+    } else {
+      NS_ADDREF(*aURI);
+    }
     return PR_TRUE;
   }
   
@@ -3108,13 +3120,20 @@ nsGenericHTMLElement::SetHrefToURI(nsIURI* aURI)
   nsCAutoString newHref;
   aURI->GetSpec(newHref);
   SetAttrHelper(nsGkAtoms::href, NS_ConvertUTF8toUTF16(newHref));
+  const nsAttrValue* attr = mAttrsAndChildren.GetAttr(nsGkAtoms::href);
+  // Might already have a URI value, if we didn't actually change the
+  // string value of our attribute.
+  if (attr && attr->Type() == nsAttrValue::eLazyURIValue &&
+      !attr->GetURIValue()) {
+    const_cast<nsAttrValue*>(attr)->CacheURIValue(aURI);
+  }
 }
 
 nsresult
 nsGenericHTMLElement::SetProtocolInHrefURI(const nsAString &aProtocol)
 {
   nsCOMPtr<nsIURI> uri;
-  GetHrefURIForAnchors(getter_AddRefs(uri));
+  GetHrefURIToMutate(getter_AddRefs(uri));
   if (!uri) {
     // Ignore failures to be compatible with NS4
     return NS_OK;
@@ -3135,7 +3154,7 @@ nsresult
 nsGenericHTMLElement::SetHostnameInHrefURI(const nsAString &aHostname)
 {
   nsCOMPtr<nsIURI> uri;
-  GetHrefURIForAnchors(getter_AddRefs(uri));
+  GetHrefURIToMutate(getter_AddRefs(uri));
   if (!uri) {
     // Ignore failures to be compatible with NS4
     return NS_OK;
@@ -3151,7 +3170,7 @@ nsresult
 nsGenericHTMLElement::SetPathnameInHrefURI(const nsAString &aPathname)
 {
   nsCOMPtr<nsIURI> uri;
-  GetHrefURIForAnchors(getter_AddRefs(uri));
+  GetHrefURIToMutate(getter_AddRefs(uri));
   nsCOMPtr<nsIURL> url = do_QueryInterface(uri);
   if (!url) {
     // Ignore failures to be compatible with NS4
@@ -3173,7 +3192,7 @@ nsGenericHTMLElement::SetHostInHrefURI(const nsAString &aHost)
   // And can't call SetHostPort, because that's not implemented.  Very sad.
   
   nsCOMPtr<nsIURI> uri;
-  GetHrefURIForAnchors(getter_AddRefs(uri));
+  GetHrefURIToMutate(getter_AddRefs(uri));
   if (!uri) {
     // Ignore failures to be compatible with NS4
     return NS_OK;
@@ -3206,7 +3225,7 @@ nsresult
 nsGenericHTMLElement::SetSearchInHrefURI(const nsAString &aSearch)
 {
   nsCOMPtr<nsIURI> uri;
-  GetHrefURIForAnchors(getter_AddRefs(uri));
+  GetHrefURIToMutate(getter_AddRefs(uri));
   nsCOMPtr<nsIURL> url = do_QueryInterface(uri);
   if (!url) {
     // Ignore failures to be compatible with NS4
@@ -3223,7 +3242,7 @@ nsresult
 nsGenericHTMLElement::SetHashInHrefURI(const nsAString &aHash)
 {
   nsCOMPtr<nsIURI> uri;
-  GetHrefURIForAnchors(getter_AddRefs(uri));
+  GetHrefURIToMutate(getter_AddRefs(uri));
   nsCOMPtr<nsIURL> url = do_QueryInterface(uri);
   if (!url) {
     // Ignore failures to be compatible with NS4
@@ -3240,7 +3259,7 @@ nsresult
 nsGenericHTMLElement::SetPortInHrefURI(const nsAString &aPort)
 {
   nsCOMPtr<nsIURI> uri;
-  GetHrefURIForAnchors(getter_AddRefs(uri));
+  GetHrefURIToMutate(getter_AddRefs(uri));
   if (!uri) {
     // Ignore failures to be compatible with NS4
     return NS_OK;

content/html/content/src/nsGenericHTMLElement.h

@@ -203,6 +203,11 @@ public:
   // Used by A, AREA, LINK, and STYLE.
   nsresult GetHrefURIForAnchors(nsIURI** aURI) const;
 
+  // As above, but makes sure to return a URI object that we can mutate with
+  // impunity without changing our current URI.  That is, if the URI is cached
+  // it clones it and returns the clone.
+  void GetHrefURIToMutate(nsIURI** aURI);
+
   // HTML element methods
   void Compact() { mAttrsAndChildren.Compact(); }
   const nsAttrValue* GetParsedAttr(nsIAtom* aAttr) const
@@ -689,12 +694,15 @@ protected:
 
   /**
    * Helper for GetURIAttr and GetHrefURIForAnchors which returns an
-   * nsIURI in the out param..
+   * nsIURI in the out param.
+   *
+   * @param aCloneIfCached if true, clone the URI before returning if
+   * it's cached.
    *
    * @return PR_TRUE if we had the attr, PR_FALSE otherwise.
    */
   NS_HIDDEN_(PRBool) GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr,
-                                nsIURI** aURI) const;
+                                PRBool aCloneIfCached, nsIURI** aURI) const;
 
   /**
    * This method works like GetURIAttr, except that it supports multiple

content/html/content/test/test_bug481335.xhtml

@@ -39,10 +39,28 @@ is($("t").href,
 var unvisitedColor = document.defaultView.getComputedStyle($("t"), "").color;
 var visitedColor;
 
+function afterThirdLoad() {
+  is(document.defaultView.getComputedStyle($("t"), "").color, visitedColor,
+     "Should be visited now after third load");
+  SimpleTest.finish();
+}
+
 function afterSecondLoad() {
   is(document.defaultView.getComputedStyle($("t"), "").color, visitedColor,
        "Should be visited now");
-  SimpleTest.finish();
+  $("t").pathname = rand;
+  is(document.defaultView.getComputedStyle($("t"), "").color, visitedColor,
+       "Should still be visited after setting pathname to its existing value");
+  dump('aaa');
+  $("t").pathname += "x";
+  isnot(document.defaultView.getComputedStyle($("t"), "").color, visitedColor,
+       "Should not be visited after changing pathname");
+  $("t").pathname = $("t").pathname;
+  isnot(document.defaultView.getComputedStyle($("t"), "").color, visitedColor,
+       "Should not be visited after setting unvisited pathname to existing value");
+
+  $("i").onload = afterThirdLoad;
+  $("i").src = $("t").href;
 }
 
 function afterFirstLoad() {