Bug 608198: nsDefaultURIFixup::KeywordToURI should always strip leading "?" from keywords, r=bz, a=blocking
authorGavin Sharp <gavin@gavinsharp.com>
Sun, 31 Oct 2010 16:01:28 -0400
changeset 57923 4c55c33b49d32c599ebf394e08ae50bc410ddff6
parent 57922 44bbb8e7929c2195c841ace4d16006036888d487
child 57924 6e4659bdc601d58d277772b3a4fa93598f6e9c5d
push idunknown
push userunknown
push dateunknown
reviewersbz, blocking
bugs608198
milestone2.0b8pre
Bug 608198: nsDefaultURIFixup::KeywordToURI should always strip leading "?" from keywords, r=bz, a=blocking
browser/base/content/test/browser_keywordSearch.js
docshell/base/nsDefaultURIFixup.cpp
--- a/browser/base/content/test/browser_keywordSearch.js
+++ b/browser/base/content/test/browser_keywordSearch.js
@@ -1,60 +1,112 @@
 /**
  * Any copyright is dedicated to the Public Domain.
  * 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");
         let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
         win.addEventListener("load", function() {
           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", {});
 }
--- a/docshell/base/nsDefaultURIFixup.cpp
+++ b/docshell/base/nsDefaultURIFixup.cpp
@@ -349,39 +349,29 @@ nsDefaultURIFixup::CreateFixupURI(const 
         KeywordToURI(aStringURI, aURI);
         if(*aURI)
             return NS_OK;
     }
 
     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", 
                                  NS_GET_IID(nsIPrefLocalizedString),
                                  getter_AddRefs(keywordURL));
 
     if (keywordURL) {
         nsXPIDLString wurl;
@@ -389,20 +379,23 @@ NS_IMETHODIMP nsDefaultURIFixup::Keyword
         CopyUTF16toUTF8(wurl, url);
     } else {
         // Fall back to a non-localized pref, for backwards compat
         mPrefBranch->GetCharPref("keyword.URL", getter_Copies(url));
     }
 
     // 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);
     }
 
 #ifdef MOZ_TOOLKIT_SEARCH
     // Try falling back to the search service's default search engine
     nsCOMPtr<nsIBrowserSearchService> searchSvc = do_GetService("@mozilla.org/browser/search-service;1");
     if (searchSvc) {
@@ -410,23 +403,23 @@ NS_IMETHODIMP nsDefaultURIFixup::Keyword
         searchSvc->GetOriginalDefaultEngine(getter_AddRefs(defaultEngine));
         if (defaultEngine) {
             nsCOMPtr<nsISearchSubmission> submission;
             // We want to allow default search plugins to specify alternate
             // parameters that are specific to keyword searches. For the moment,
             // 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));
             }
 
             if (submission) {
                 // The submission depends on POST data (i.e. the search engine's
                 // "method" is POST), we can't use this engine for keyword
                 // searches