Bug 1009648: nsStandardURL crashes when calling SetHost after Clear() r=mcmanus

netwerk/base/src/nsStandardURL.cpp

@@ -1135,10 +1135,13 @@ nsStandardURL::SetSpec(const nsACString &input)
 
     LOG(("nsStandardURL::SetSpec [spec=%s]\n", spec));
 
-    Clear();
-
     if (!spec || !*spec)
-        return NS_OK;
+        return NS_ERROR_MALFORMED_URI;
+
+    // Make a backup of the curent URL
+    nsStandardURL prevURL;
+    prevURL.CopyMembers(this, eHonorRef);
+    Clear();
 
     // filter out unexpected chars "\r\n\t" if necessary
     nsAutoCString buf1;
@@ -1157,6 +1160,9 @@ nsStandardURL::SetSpec(const nsACString &input)
 
     if (NS_FAILED(rv)) {
         Clear();
+        // If parsing the spec has failed, restore the old URL
+        // so we don't end up with an empty URL.
+        CopyMembers(&prevURL, eHonorRef);
         return rv;
     }
 
@@ -1835,39 +1841,57 @@ nsStandardURL::CloneInternal(nsStandardURL::RefHandlingEnum refHandlingMode,
     if (!clone)
         return NS_ERROR_OUT_OF_MEMORY;
 
-    clone->mSpec = mSpec;
-    clone->mDefaultPort = mDefaultPort;
-    clone->mPort = mPort;
-    clone->mScheme = mScheme;
-    clone->mAuthority = mAuthority;
-    clone->mUsername = mUsername;
-    clone->mPassword = mPassword;
-    clone->mHost = mHost;
-    clone->mPath = mPath;
-    clone->mFilepath = mFilepath;
-    clone->mDirectory = mDirectory;
-    clone->mBasename = mBasename;
-    clone->mExtension = mExtension;
-    clone->mQuery = mQuery;
-    clone->mRef = mRef;
-    clone->mOriginCharset = mOriginCharset;
-    clone->mURLType = mURLType;
-    clone->mParser = mParser;
-    clone->mFile = mFile;
-    clone->mHostA = mHostA ? strdup(mHostA) : nullptr;
-    clone->mMutable = true;
-    clone->mSupportsFileURL = mSupportsFileURL;
-    clone->mHostEncoding = mHostEncoding;
-    clone->mSpecEncoding = mSpecEncoding;
-
-    if (refHandlingMode == eIgnoreRef) {
-        clone->SetRef(EmptyCString());
-    }
+    // Copy local members into clone.
+    // Also copies the cached members mFile, mHostA
+    clone->CopyMembers(this, refHandlingMode, true);
 
     clone.forget(result);
     return NS_OK;
 }
 
+nsresult nsStandardURL::CopyMembers(nsStandardURL * source,
+    nsStandardURL::RefHandlingEnum refHandlingMode, bool copyCached)
+{
+    mSpec = source->mSpec;
+    mDefaultPort = source->mDefaultPort;
+    mPort = source->mPort;
+    mScheme = source->mScheme;
+    mAuthority = source->mAuthority;
+    mUsername = source->mUsername;
+    mPassword = source->mPassword;
+    mHost = source->mHost;
+    mPath = source->mPath;
+    mFilepath = source->mFilepath;
+    mDirectory = source->mDirectory;
+    mBasename = source->mBasename;
+    mExtension = source->mExtension;
+    mQuery = source->mQuery;
+    mRef = source->mRef;
+    mOriginCharset = source->mOriginCharset;
+    mURLType = source->mURLType;
+    mParser = source->mParser;
+    mMutable = true;
+    mSupportsFileURL = source->mSupportsFileURL;
+    mHostEncoding = source->mHostEncoding;
+
+    if (copyCached) {
+        mFile = source->mFile;
+        mHostA = source->mHostA ? strdup(source->mHostA) : nullptr;
+        mSpecEncoding = source->mSpecEncoding;
+    } else {
+        // The same state as after calling InvalidateCache()
+        mFile = nullptr;
+        mHostA = nullptr;
+        mSpecEncoding = eEncoding_Unknown;
+    }
+
+    if (refHandlingMode == eIgnoreRef) {
+        SetRef(EmptyCString());
+    }
+
+    return NS_OK;
+}
+
 NS_IMETHODIMP
 nsStandardURL::Resolve(const nsACString &in, nsACString &out)
 {

netwerk/base/src/nsStandardURL.h

@@ -156,6 +156,10 @@ protected:
     // Helper to share code between Clone methods.
     nsresult CloneInternal(RefHandlingEnum aRefHandlingMode,
                            nsIURI** aClone);
+    // Helper method that copies member variables from the source StandardURL
+    // if copyCached = true, it will also copy mFile and mHostA
+    nsresult CopyMembers(nsStandardURL * source, RefHandlingEnum mode,
+                         bool copyCached = false);
 
     // Helper for subclass implementation of GetFile().  Subclasses that map
     // URIs to files in a special way should implement this method.  It should

netwerk/test/unit/test_standardurl.js

@@ -208,6 +208,18 @@ function test_ipv6_fail()
   Assert.throws(() => { url.hostPort = "[2001::1]:bad"; }, "bad port number");
 }
 
+function test_clearedSpec()
+{
+  var url = stringToURL("http://example.com/path");
+  Assert.throws(() => { url.spec = "http: example"; }, "set bad spec");
+  Assert.throws(() => { url.spec = ""; }, "set empty spec");
+  do_check_eq(url.spec, "http://example.com/path");
+  url.host = "allizom.org";
+
+  var ref = stringToURL("http://allizom.org/path");
+  symmetricEquality(true, url, ref);
+}
+
 function run_test()
 {
   test_setEmptyPath();
@@ -215,4 +227,5 @@ function run_test()
   test_setRef();
   test_ipv6();
   test_ipv6_fail();
+  test_clearedSpec();
 }