Bug 608198: nsDefaultURIFixup::KeywordToURI should always strip leading "?" from keywords, r=bz, a=blocking

browser/base/content/test/browser_keywordSearch.js

@@ -3,37 +3,41 @@
  * http://creativecommons.org/publicdomain/zero/1.0/
  **/
 
+var gTests = [
+  {
+    name: "normal search (search service)",
+    testText: "test search",
+    searchURL: Services.search.originalDefaultEngine.getSubmission("test search").uri.spec
+  },
+  {
+    name: "?-prefixed search (search service)",
+    testText: "?   foo  ",
+    searchURL: Services.search.originalDefaultEngine.getSubmission("foo").uri.spec
+  },
+  {
+    name: "normal search (keyword.url)",
+    testText: "test search",
+    keywordURLPref: "http://example.com/?q=",
+    searchURL: "http://example.com/?q=test+search"
+  },
+  {
+    name: "?-prefixed search (keyword.url)",
+    testText: "?   foo  ",
+    keywordURLPref: "http://example.com/?q=",
+    searchURL: "http://example.com/?q=foo"
+  },
+  {
+    name: "encoding test (keyword.url)",
+    testText: "test encoded+%/",
+    keywordURLPref: "http://example.com/?q=",
+    searchURL: "http://example.com/?q=test+encoded%2B%25%2F"
+  }
+];
+
 function test() {
   waitForExplicitFinish();
 
-  let tab = gBrowser.selectedTab = gBrowser.addTab();
-  let searchText = "test search";
-
-  let listener = {
-    onStateChange: function onLocationChange(webProgress, req, flags, status) {
-      ok(flags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT, "only notified for document");
-
-      // Only care about starts
-      if (!(flags & Ci.nsIWebProgressListener.STATE_START))
-        return;
-
-      ok(req instanceof Ci.nsIChannel, "req is a channel");
-
-      let searchURL = Services.search.originalDefaultEngine.getSubmission(searchText).uri.spec;
-      is(req.originalURI.spec, searchURL, "search URL was loaded");
-      info("Actual URI: " + req.URI.spec);
-
-      Services.ww.unregisterNotification(observer);
-      gBrowser.removeProgressListener(this);
-      executeSoon(function () {
-        gBrowser.removeTab(tab);
-        finish();
-      });
-    }
-  }
-  gBrowser.addProgressListener(listener, Ci.nsIWebProgressListener.NOTIFY_STATE_DOCUMENT);
-
-  let observer = {
+  let windowObserver = {
     observe: function(aSubject, aTopic, aData) {
       if (aTopic == "domwindowopened") {
         ok(false, "Alert window opened");
@@ -42,19 +46,67 @@ function test() {
           win.removeEventListener("load", arguments.callee, false);
           win.close();
         }, false);
-        gBrowser.removeProgressListener(listener);
-        executeSoon(function () {
-          gBrowser.removeTab(tab);
-          finish();
-        });
+        executeSoon(finish);
       }
-      Services.ww.unregisterNotification(this);
     }
   };
-  Services.ww.registerNotification(observer);
+
+  Services.ww.registerNotification(windowObserver);
+
+  let tab = gBrowser.selectedTab = gBrowser.addTab();
+
+  let listener = {
+    onStateChange: function onLocationChange(webProgress, req, flags, status) {
+      // Only care about document starts
+      let docStart = Ci.nsIWebProgressListener.STATE_IS_DOCUMENT |
+                     Ci.nsIWebProgressListener.STATE_START;
+      if (!(flags & docStart))
+        return;
+
+      info("received document start");
+
+      ok(req instanceof Ci.nsIChannel, "req is a channel");
+      is(req.originalURI.spec, gCurrTest.searchURL, "search URL was loaded");
+      info("Actual URI: " + req.URI.spec);
+
+      executeSoon(nextTest);
+    }
+  }
+  gBrowser.addProgressListener(listener);
+
+  registerCleanupFunction(function () {
+    Services.ww.unregisterNotification(windowObserver);
+
+    gBrowser.removeProgressListener(listener);
+    gBrowser.removeTab(tab);
+  });
+
+  nextTest();
+}
+
+var gCurrTest;
+function nextTest() {
+  // Clear the pref before every test (and after the last)
+  try {
+    Services.prefs.clearUserPref("keyword.URL");
+  } catch(ex) {}
+
+  if (gTests.length) {
+    gCurrTest = gTests.shift();
+    doTest();
+  } else {
+    finish();
+  }
+}
+
+function doTest() {
+  info("Running test: " + gCurrTest.name);
+
+  if (gCurrTest.keywordURLPref)
+    Services.prefs.setCharPref("keyword.URL", gCurrTest.keywordURLPref);
 
   // Simulate a user entering search terms
-  gURLBar.value = searchText;
+  gURLBar.value = gCurrTest.testText;
   gURLBar.focus();
   EventUtils.synthesizeKey("VK_RETURN", {});
 }

docshell/base/nsDefaultURIFixup.cpp

@@ -354,29 +354,19 @@ nsDefaultURIFixup::CreateFixupURI(const nsACString& aStringURI, PRUint32 aFixupF
     return rv;
 }
 
-static nsresult MangleKeywordIntoURI(const char *aKeyword, const char *aURL,
-                                     nsCString& query)
-{
-    query = (*aKeyword == '?') ? (aKeyword + 1) : aKeyword;
-    query.Trim(" "); // pull leading/trailing spaces.
-
-    // encode
-    char * encQuery = nsEscape(query.get(), url_XPAlphas);
-    if (!encQuery) return NS_ERROR_OUT_OF_MEMORY;
-    query.Adopt(encQuery);
-
-    // prepend the query with the keyword url
-    // XXX this url should come from somewhere else
-    query.Insert(aURL, 0);
-    return NS_OK;
-}
-
 NS_IMETHODIMP nsDefaultURIFixup::KeywordToURI(const nsACString& aKeyword,
                                               nsIURI **aURI)
 {
     *aURI = nsnull;
     NS_ENSURE_STATE(mPrefBranch);
 
+    // Strip leading "?" and leading/trailing spaces from aKeyword
+    nsCAutoString keyword(aKeyword);
+    if (StringBeginsWith(keyword, NS_LITERAL_CSTRING("?"))) {
+        keyword.Cut(0, 1);
+    }
+    keyword.Trim(" ");
+
     nsXPIDLCString url;
     nsCOMPtr<nsIPrefLocalizedString> keywordURL;
     mPrefBranch->GetComplexValue("keyword.URL", 
@@ -394,10 +384,13 @@ NS_IMETHODIMP nsDefaultURIFixup::KeywordToURI(const nsACString& aKeyword,
 
     // If the pref is set and non-empty, use it.
     if (!url.IsEmpty()) {
+        // Escape keyword, then prepend URL
         nsCAutoString spec;
-        nsresult rv = MangleKeywordIntoURI(PromiseFlatCString(aKeyword).get(),
-                                           url.get(), spec);
-        if (NS_FAILED(rv)) return rv;
+        if (!NS_Escape(keyword, spec, url_XPAlphas)) {
+            return NS_ERROR_OUT_OF_MEMORY;
+        }
+
+        spec.Insert(url, 0);
 
         return NS_NewURI(aURI, spec);
     }
@@ -415,13 +408,13 @@ NS_IMETHODIMP nsDefaultURIFixup::KeywordToURI(const nsACString& aKeyword,
             // do this by first looking for a magic
             // "application/x-moz-keywordsearch" submission type. In the future,
             // we should instead use a solution that relies on bug 587780.
-            defaultEngine->GetSubmission(NS_ConvertUTF8toUTF16(aKeyword),
+            defaultEngine->GetSubmission(NS_ConvertUTF8toUTF16(keyword),
                                          NS_LITERAL_STRING("application/x-moz-keywordsearch"),
                                          getter_AddRefs(submission));
             // If getting the special x-moz-keywordsearch submission type failed,
             // fall back to the default response type.
             if (!submission) {
-                defaultEngine->GetSubmission(NS_ConvertUTF8toUTF16(aKeyword),
+                defaultEngine->GetSubmission(NS_ConvertUTF8toUTF16(keyword),
                                              EmptyString(),
                                              getter_AddRefs(submission));
             }