fix bookmarks API crash when handling invalid URL

also re-enable test on Windows (can't repro crashes)

BUG=56945,45595
TEST=ExtensionApiTest.Bookmarks

Review URL: http://codereview.chromium.org/3672003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62170 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 37 insertions, 23 deletions

diff --git a/chrome/browser/extensions/extension_bookmarks_apitest.cc b/chrome/browser/extensions/extension_bookmarks_apitest.cc
index a0e2c01..8af2971 100644
--- a/chrome/browser/extensions/extension_bookmarks_apitest.cc
+++ b/chrome/browser/extensions/extension_bookmarks_apitest.cc
@@ -4,13 +4,6 @@
 
 #include "chrome/browser/extensions/extension_apitest.h"
 
-#if defined(OS_WIN)
-// On Windows, this test occasionally crash. See http://crbug.com/45595
-#define MAYBE_Bookmarks DISABLED_Bookmarks
-#else
-#define MAYBE_Bookmarks Bookmarks
-#endif
-
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, MAYBE_Bookmarks) {
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Bookmarks) {
   ASSERT_TRUE(RunExtensionTest("bookmarks")) << message_;
 }
diff --git a/chrome/browser/extensions/extension_bookmarks_module.cc b/chrome/browser/extensions/extension_bookmarks_module.cc
index 991ea2d..1f6e4ee 100644
--- a/chrome/browser/extensions/extension_bookmarks_module.cc
+++ b/chrome/browser/extensions/extension_bookmarks_module.cc
@@ -545,20 +545,17 @@ bool UpdateBookmarkFunction::RunImpl() {
   DictionaryValue* updates;
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &updates));
 
-  string16 title;
-  std::string url_string;
-
   // Optional but we need to distinguish non present from an empty title.
+  string16 title;
   const bool has_title = updates->GetString(keys::kTitleKey, &title);
-  updates->GetString(keys::kUrlKey, &url_string);  // Optional.
-
-  GURL url;
-  if (!url_string.empty()) {
-    url = GURL(url_string);
 
-    // If URL is present then it needs to be a non empty valid URL.
-    EXTENSION_FUNCTION_VALIDATE(!url.is_empty());
-    EXTENSION_FUNCTION_VALIDATE(url.is_valid());
+  // Optional.
+  std::string url_string;
+  updates->GetString(keys::kUrlKey, &url_string);
+  GURL url(url_string);
+  if (!url_string.empty() && !url.is_valid()) {
+    error_ = keys::kInvalidUrlError;
+    return false;
   }
 
   BookmarkModel* model = profile()->GetBookmarkModel();
diff --git a/chrome/renderer/resources/extension_apitest.js b/chrome/renderer/resources/extension_apitest.js
index ca7a4df..4b6f939 100644
--- a/chrome/renderer/resources/extension_apitest.js
+++ b/chrome/renderer/resources/extension_apitest.js
@@ -173,6 +173,8 @@ var chrome = chrome || {};
         chrome.test.assertNoLastError();
       } else {
         chrome.test.assertEq(typeof(expectedError), 'string');
+        chrome.test.assertTrue(chrome.extension.lastError != undefined,
+            "No lastError, but expected " + expectedError);
         chrome.test.assertEq(expectedError, chrome.extension.lastError.message);
       }
 
@@ -214,8 +216,8 @@ var chrome = chrome || {};
     return chrome.test.callback(func);
   };
 
-  chrome.test.callbackFail = function(expectedError) {
-    return chrome.test.callback(null, expectedError);
+  chrome.test.callbackFail = function(expectedError, func) {
+    return chrome.test.callback(func, expectedError);
   };
 
   chrome.test.runTests = function(tests) {
diff --git a/chrome/test/data/extensions/api_test/bookmarks/test.js b/chrome/test/data/extensions/api_test/bookmarks/test.js
index ce8db8c..13eb646 100644
--- a/chrome/test/data/extensions/api_test/bookmarks/test.js
+++ b/chrome/test/data/extensions/api_test/bookmarks/test.js
@@ -222,7 +222,7 @@ chrome.test.runTests([
       node3.parentId = results.parentId;
       node3.index = 1;
       node2.index = 2;
-      
+
       // update expected to match
       expected[0].children[0].children.pop();
       expected[0].children[0].children.pop();
@@ -283,11 +283,33 @@ chrome.test.runTests([
       chrome.test.assertEq(title, results.title);
     }));
 
-    var url = "http://example.com/hello"
+    var url = "http://example.com/hello";
     chrome.bookmarks.update(node1.id, {"url": url}, pass(function(results) {
       // Make sure that leaving out the title does not set the title to empty.
       chrome.test.assertEq(title, results.title);
       chrome.test.assertEq(url, results.url);
+
+      // Empty or invalid URLs should not change the URL.
+      var bad_url = "";
+      chrome.bookmarks.update(node1.id, {"url": bad_url},
+        pass(function(results) {
+          chrome.bookmarks.get(node1.id, pass(function(results) {
+            chrome.test.assertEq(url, results[0].url);
+            chrome.test.log("URL UNCHANGED");
+          }));
+        })
+      );
+
+      // Invalid URLs also generate an error.
+      bad_url = "I am not an URL";
+      chrome.bookmarks.update(node1.id, {"url": bad_url}, fail("Invalid URL.",
+        function(results) {
+          chrome.bookmarks.get(node1.id, pass(function(results) {
+            chrome.test.assertEq(url, results[0].url);
+            chrome.test.log("URL UNCHANGED");
+          }));
+        })
+      );
     }));
   },