Revert 114515 - broke CrOS tests:

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromeOS/builds/2495/steps/unit_tests/logs/stdio

Show parse errors in the UI when loading ONC files.
BUG=chromium-os:23757
TEST=manual
Review URL: http://codereview.chromium.org/8883046

TBR=chocobo@chromium.org
Review URL: http://codereview.chromium.org/8947002

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

10 files changed, 37 insertions, 108 deletions

diff --git a/chrome/browser/chromeos/cros/mock_network_library.h b/chrome/browser/chromeos/cros/mock_network_library.h
index 293149b..aa0b469 100644
--- a/chrome/browser/chromeos/cros/mock_network_library.h
+++ b/chrome/browser/chromeos/cros/mock_network_library.h
@@ -155,9 +155,7 @@ class MockNetworkLibrary : public NetworkLibrary {
                                                    HardwareAddressFormat));
   MOCK_METHOD1(SetIPConfig, void(const NetworkIPConfig&));
   MOCK_METHOD0(SwitchToPreferredNetwork, void(void));
-  MOCK_METHOD3(LoadOncNetworks, bool(const std::string&,
-                                     const std::string&,
-                                     std::string*));
+  MOCK_METHOD2(LoadOncNetworks, bool(const std::string&, const std::string&));
   MOCK_METHOD2(SetActiveNetwork, bool(ConnectionType, const std::string&));
 };
 
diff --git a/chrome/browser/chromeos/cros/network_library.cc b/chrome/browser/chromeos/cros/network_library.cc
index d1625d1..43acb7c 100644
--- a/chrome/browser/chromeos/cros/network_library.cc
+++ b/chrome/browser/chromeos/cros/network_library.cc
@@ -1745,8 +1745,7 @@ class NetworkLibraryImplBase : public NetworkLibrary  {
   // virtual SetIPConfig implemented in derived classes.
   virtual void SwitchToPreferredNetwork() OVERRIDE;
   virtual bool LoadOncNetworks(const std::string& onc_blob,
-                               const std::string& passcode,
-                               std::string* error) OVERRIDE;
+                               const std::string& passcode) OVERRIDE;
   virtual bool SetActiveNetwork(ConnectionType type,
                                 const std::string& service_path) OVERRIDE;
 
@@ -2835,23 +2834,14 @@ void NetworkLibraryImplBase::SwitchToPreferredNetwork() {
 }
 
 bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
-                                             const std::string& passcode,
-                                             std::string* error) {
+                                             const std::string& passcode) {
   // TODO(gspencer): Add support for decrypting onc files. crbug.com/19397
   OncNetworkParser parser(onc_blob);
 
-  if (!parser.parse_error().empty()) {
-    if (error)
-      *error = parser.parse_error();
-    return false;
-  }
-
   for (int i = 0; i < parser.GetCertificatesSize(); i++) {
     // Insert each of the available certs into the certificate DB.
     if (parser.ParseCertificate(i).get() == NULL) {
       DLOG(WARNING) << "Cannot parse certificate in ONC file";
-      if (error)
-        *error = parser.parse_error();
       return false;
     }
   }
@@ -2861,8 +2851,6 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
     scoped_ptr<Network> network(parser.ParseNetwork(i));
     if (!network.get()) {
       DLOG(WARNING) << "Cannot parse network in ONC file";
-      if (error)
-        *error = parser.parse_error();
       return false;
     }
 
@@ -2880,15 +2868,8 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
 
     CallConfigureService(network->unique_id(), &dict);
   }
-
-  if (parser.GetNetworkConfigsSize() != 0 ||
-      parser.GetCertificatesSize() != 0) {
-      if (error)
-        *error = l10n_util::GetStringUTF8(
-            IDS_NETWORK_CONFIG_ERROR_NETWORK_IMPORT);
-      return false;
-  }
-  return true;
+  return (parser.GetNetworkConfigsSize() != 0 ||
+          parser.GetCertificatesSize() != 0);
 }
 
 ////////////////////////////////////////////////////////////////////////////
@@ -5121,7 +5102,7 @@ void NetworkLibraryImplStub::Init() {
         "  ],"
         "  \"Certificates\": []"
         "}");
-  LoadOncNetworks(test_blob, "", NULL);
+  LoadOncNetworks(test_blob, "");
 }
 
 ////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/chromeos/cros/network_library.h b/chrome/browser/chromeos/cros/network_library.h
index 5fe3a90..b27f4db 100644
--- a/chrome/browser/chromeos/cros/network_library.h
+++ b/chrome/browser/chromeos/cros/network_library.h
@@ -1789,11 +1789,8 @@ class NetworkLibrary {
   virtual void SwitchToPreferredNetwork() = 0;
 
   // Load networks from an Open Network Configuration blob.
-  // If there was an error, this will return false and |error| will be set to
-  // the error message.
   virtual bool LoadOncNetworks(const std::string& onc_blob,
-                               const std::string& passcode,
-                               std::string* error) = 0;
+                               const std::string& passcode) = 0;
 
   // This sets the active network for the network type. Note: priority order
   // is unchanged (i.e. if a wifi network is set to active, but an ethernet
diff --git a/chrome/browser/chromeos/cros/onc_network_parser.cc b/chrome/browser/chromeos/cros/onc_network_parser.cc
index eedf109..670d08a 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser.cc
@@ -16,13 +16,11 @@
 #include "chrome/browser/chromeos/cros/native_network_parser.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/common/net/x509_certificate_model.h"
-#include "grit/generated_resources.h"
 #include "net/base/cert_database.h"
 #include "net/base/crypto_module.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_certificate.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
-#include "ui/base/l10n/l10n_util.h"
 
 namespace chromeos {
 
@@ -237,13 +235,10 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
     int cert_index) {
   CHECK(certificates_);
   CHECK(static_cast<size_t>(cert_index) < certificates_->GetSize());
-  CHECK_GE(cert_index, 0);
+  CHECK(cert_index >= 0);
   base::DictionaryValue* certificate = NULL;
-  if (!certificates_->GetDictionary(cert_index, &certificate)) {
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
-    return NULL;
-  }
+  certificates_->GetDictionary(cert_index, &certificate);
+  CHECK(certificate);
 
   if (VLOG_IS_ON(2)) {
     std::string certificate_json;
@@ -259,8 +254,6 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
   if (!certificate->GetString("GUID", &guid) || guid.empty()) {
     LOG(WARNING) << "ONC File: certificate missing identifier at index"
                  << cert_index;
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_GUID_MISSING);
     return NULL;
   }
 
@@ -269,10 +262,9 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
 
   net::CertDatabase cert_database;
   if (remove) {
-    if (!DeleteCertAndKeyByNickname(guid)) {
-      parse_error_ = l10n_util::GetStringUTF8(
-          IDS_NETWORK_CONFIG_ERROR_CERT_DELETE);
-    }
+    bool success = DeleteCertAndKeyByNickname(guid);
+    DCHECK(success);
+    // TODO(gspencer): return removed certificate?
     return NULL;
   }
 
@@ -288,22 +280,15 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
 
   LOG(WARNING) << "ONC File: certificate of unknown type: " << cert_type
                << " at index " << cert_index;
-  parse_error_ = l10n_util::GetStringUTF8(
-      IDS_NETWORK_CONFIG_ERROR_CERT_TYPE_MISSING);
   return NULL;
 }
 
 Network* OncNetworkParser::ParseNetwork(int n) {
-  CHECK(network_configs_);
-  CHECK(static_cast<size_t>(n) < network_configs_->GetSize());
-  CHECK_GE(n, 0);
+  if (!network_configs_)
+    return NULL;
   DictionaryValue* info = NULL;
-  if (!network_configs_->GetDictionary(n, &info)) {
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
+  if (!network_configs_->GetDictionary(n, &info))
     return NULL;
-  }
-
   if (VLOG_IS_ON(2)) {
     std::string network_json;
     base::JSONWriter::Write(static_cast<base::Value*>(info),
@@ -319,11 +304,8 @@ Network* OncNetworkParser::CreateNetworkFromInfo(
     const std::string& service_path,
     const DictionaryValue& info) {
   ConnectionType type = ParseTypeFromDictionary(info);
-  if (type == TYPE_UNKNOWN) {  // Return NULL if cannot parse network type.
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_NETWORK_TYPE_MISSING);
+  if (type == TYPE_UNKNOWN)  // Return NULL if cannot parse network type.
     return NULL;
-  }
   scoped_ptr<Network> network(CreateNewNetwork(type, service_path));
   if (!ParseNestedObject(network.get(),
                          "NetworkConfiguration",
@@ -331,9 +313,6 @@ Network* OncNetworkParser::CreateNetworkFromInfo(
                          network_configuration_signature,
                          ParseNetworkConfigurationValue)) {
     LOG(WARNING) << "Network " << network->name() << " failed to parse.";
-    if (parse_error_.empty())
-      parse_error_ = l10n_util::GetStringUTF8(
-          IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
     return NULL;
   }
   if (VLOG_IS_ON(2)) {
@@ -394,8 +373,6 @@ OncNetworkParser::ParseServerOrCaCertificate(
       if (!trust_list->GetString(i, &trust_type)) {
         LOG(WARNING) << "ONC File: certificate trust is invalid at index "
                      << cert_index;
-        parse_error_ = l10n_util::GetStringUTF8(
-            IDS_NETWORK_CONFIG_ERROR_CERT_TRUST_INVALID);
         return NULL;
       }
       if (trust_type == "Web") {
@@ -404,8 +381,6 @@ OncNetworkParser::ParseServerOrCaCertificate(
         LOG(WARNING) << "ONC File: certificate contains unknown "
                      << "trust type: " << trust_type
                      << " at index " << cert_index;
-        parse_error_ = l10n_util::GetStringUTF8(
-            IDS_NETWORK_CONFIG_ERROR_CERT_TRUST_UNKNOWN);
         return NULL;
       }
     }
@@ -416,8 +391,6 @@ OncNetworkParser::ParseServerOrCaCertificate(
     LOG(WARNING) << "ONC File: certificate missing appropriate "
                  << "certificate data for type: " << cert_type
                  << " at index " << cert_index;
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MISSING);
     return NULL;
   }
 
@@ -425,8 +398,6 @@ OncNetworkParser::ParseServerOrCaCertificate(
   if (!base::Base64Decode(x509_data, &decoded_x509)) {
     LOG(WARNING) << "Unable to base64 decode X509 data: \""
                  << x509_data << "\".";
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
     return NULL;
   }
 
@@ -437,8 +408,6 @@ OncNetworkParser::ParseServerOrCaCertificate(
           guid.c_str());
   if (!x509_cert.get()) {
     LOG(WARNING) << "Unable to create X509 certificate from bytes.";
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
     return NULL;
   }
 
@@ -448,7 +417,7 @@ OncNetworkParser::ParseServerOrCaCertificate(
   bool success = false;
   if (cert_type == "Server") {
     success = cert_database.ImportServerCert(cert_list, &failures);
-  } else {  // Authority cert
+  } else { // Authority cert
     net::CertDatabase::TrustBits trust = web_trust ?
                                          net::CertDatabase::TRUSTED_SSL :
                                          net::CertDatabase::UNTRUSTED;
@@ -459,15 +428,11 @@ OncNetworkParser::ParseServerOrCaCertificate(
                  << net::ErrorToString(failures[0].net_error)
                  << ") importing " << cert_type << " certificate at index "
                  << cert_index;
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_IMPORT);
     return NULL;
   }
   if (!success) {
     LOG(WARNING) << "ONC File: Unknown error importing " << cert_type
                  << " certificate at index " << cert_index;
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_UNKNOWN);
     return NULL;
   }
   VLOG(2) << "Successfully imported server/ca certificate at index "
@@ -486,8 +451,6 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseClientCertificate(
       pkcs12_data.empty()) {
     LOG(WARNING) << "ONC File: PKCS12 data is missing for Client "
                  << "certificate at index " << cert_index;
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MISSING);
     return NULL;
   }
 
@@ -495,8 +458,6 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseClientCertificate(
   if (!base::Base64Decode(pkcs12_data, &decoded_pkcs12)) {
     LOG(WARNING) << "Unable to base64 decode PKCS#12 data: \""
                  << pkcs12_data << "\".";
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
     return NULL;
   }
 
@@ -510,8 +471,6 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseClientCertificate(
     LOG(WARNING) << "ONC File: Unable to import Client certificate at index "
                  << cert_index
                  << " (error " << net::ErrorToString(result) << ").";
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_CERT_IMPORT);
     return NULL;
   }
 
@@ -556,8 +515,6 @@ bool OncNetworkParser::ParseNestedObject(Network* network,
   bool any_errors = false;
   if (!value.IsType(base::Value::TYPE_DICTIONARY)) {
     VLOG(1) << network->name() << ": expected object of type " << onc_type;
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
     return false;
   }
   VLOG(2) << "Parsing nested object of type " << onc_type;
@@ -603,9 +560,6 @@ bool OncNetworkParser::ParseNestedObject(Network* network,
               << "(" << index << ")] = " << value_json;
     }
   }
-  if (any_errors)
-    parse_error_ = l10n_util::GetStringUTF8(
-        IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
   return !any_errors;
 }
 
diff --git a/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc b/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
index 9b3ece0..ec5907a 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
@@ -263,6 +263,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifi1) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
+  EXPECT_FALSE(parser.ParseNetwork(1));
   scoped_ptr<Network> network(parser.ParseNetwork(0));
   ASSERT_TRUE(network.get());
 
@@ -298,6 +299,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifiEAP1) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
+  EXPECT_FALSE(parser.ParseNetwork(1));
   scoped_ptr<Network> network(parser.ParseNetwork(0));
   ASSERT_TRUE(network.get());
 
@@ -337,6 +339,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifiEAP2) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
+  EXPECT_FALSE(parser.ParseNetwork(1));
   scoped_ptr<Network> network(parser.ParseNetwork(0));
   ASSERT_TRUE(network.get());
 
diff --git a/chrome/browser/policy/network_configuration_updater.cc b/chrome/browser/policy/network_configuration_updater.cc
index a1597ca..788a010 100644
--- a/chrome/browser/policy/network_configuration_updater.cc
+++ b/chrome/browser/policy/network_configuration_updater.cc
@@ -56,11 +56,8 @@ void NetworkConfigurationUpdater::ApplyNetworkConfiguration(
 
   if (*cached_value != new_network_config) {
     *cached_value = new_network_config;
-    std::string error;
-    if (!network_library_->LoadOncNetworks(new_network_config, "", &error)) {
-      LOG(WARNING) << "Network library failed to load ONC configuration:"
-          << error;
-    }
+    if (!network_library_->LoadOncNetworks(new_network_config, ""))
+      LOG(WARNING) << "Network library failed to load ONC configuration.";
   }
 }
 
diff --git a/chrome/browser/policy/network_configuration_updater_unittest.cc b/chrome/browser/policy/network_configuration_updater_unittest.cc
index c38992b..d1e71a2 100644
--- a/chrome/browser/policy/network_configuration_updater_unittest.cc
+++ b/chrome/browser/policy/network_configuration_updater_unittest.cc
@@ -26,7 +26,7 @@ class NetworkConfigurationUpdaterTest
 TEST_P(NetworkConfigurationUpdaterTest, InitialUpdate) {
   provider_.AddPolicy(GetParam(), Value::CreateStringValue(kFakeONC));
 
-  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, "", NULL))
+  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, ""))
       .WillRepeatedly(Return(true));
 
   NetworkConfigurationUpdater updater(&provider_, &network_library_);
@@ -37,20 +37,20 @@ TEST_P(NetworkConfigurationUpdaterTest, PolicyChange) {
   NetworkConfigurationUpdater updater(&provider_, &network_library_);
 
   // We should update if policy changes.
-  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, "", NULL))
+  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, ""))
       .WillOnce(Return(true));
   provider_.AddPolicy(GetParam(), Value::CreateStringValue(kFakeONC));
   provider_.NotifyPolicyUpdated();
   Mock::VerifyAndClearExpectations(&network_library_);
 
   // No update if the set the same value again.
-  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, "", NULL))
+  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, ""))
       .Times(0);
   provider_.NotifyPolicyUpdated();
   Mock::VerifyAndClearExpectations(&network_library_);
 
   // Another update is expected if the policy goes away.
-  EXPECT_CALL(network_library_, LoadOncNetworks("", "", NULL))
+  EXPECT_CALL(network_library_, LoadOncNetworks("", ""))
       .WillOnce(Return(true));
   provider_.RemovePolicy(GetParam());
   provider_.NotifyPolicyUpdated();
diff --git a/chrome/browser/resources/net_internals/browser_bridge.js b/chrome/browser/resources/net_internals/browser_bridge.js
index 032b2e1..6a2d308 100644
--- a/chrome/browser/resources/net_internals/browser_bridge.js
+++ b/chrome/browser/resources/net_internals/browser_bridge.js
@@ -316,9 +316,9 @@ var BrowserBridge = (function() {
         this.hstsObservers_[i].onHSTSQueryResult(info);
     },
 
-    receivedONCFileParse: function(error) {
+    receivedONCFileParse: function(status) {
       for (var i = 0; i < this.crosONCFileParseObservers_.length; i++)
-        this.crosONCFileParseObservers_[i].onONCFileParse(error);
+        this.crosONCFileParseObservers_[i].onONCFileParse(status);
     },
 
     receivedHttpCacheInfo: function(info) {
@@ -498,7 +498,7 @@ var BrowserBridge = (function() {
      * Adds a listener for ONC file parse status. The observer will be called
      * back with:
      *
-     *   observer.onONCFileParse(error);
+     *   observer.onONCFileParse(status);
      */
     addCrosONCFileParseObserver: function(observer) {
       this.crosONCFileParseObservers_.push(observer);
diff --git a/chrome/browser/resources/net_internals/chromeos_view.js b/chrome/browser/resources/net_internals/chromeos_view.js
index a3d2a35..b560d82 100644
--- a/chrome/browser/resources/net_internals/chromeos_view.js
+++ b/chrome/browser/resources/net_internals/chromeos_view.js
@@ -20,7 +20,7 @@ var CrosView = (function() {
     if (fileContent)
       g_browser.importONCFile(fileContent, passcode);
     else
-      setParseStatus_('ONC file parse failed: cannot read file');
+      setParseStatus_(false);
   }
 
   /**
@@ -69,11 +69,11 @@ var CrosView = (function() {
    *
    *  @private
    */
-  function setParseStatus_(error) {
+  function setParseStatus_(success) {
     var parseStatus = $(CrosView.PARSE_STATUS_ID);
     parseStatus.hidden = false;
-    parseStatus.textContent = error ?
-        'ONC file parse failed: ' + error : 'ONC file successfully parsed';
+    parseStatus.textContent = success ?
+        "ONC file successfully parsed" : "ONC file parse failed";
     reset_();
   }
 
diff --git a/chrome/browser/ui/webui/net_internals_ui.cc b/chrome/browser/ui/webui/net_internals_ui.cc
index 09851c0..8660432 100644
--- a/chrome/browser/ui/webui/net_internals_ui.cc
+++ b/chrome/browser/ui/webui/net_internals_ui.cc
@@ -1295,11 +1295,10 @@ void NetInternalsMessageHandler::OnImportONCFile(const ListValue* list) {
     NOTREACHED();
   }
 
-  std::string error;
-  chromeos::CrosLibrary::Get()->GetNetworkLibrary()->
-      LoadOncNetworks(onc_blob, passcode, &error);
+  const bool success = chromeos::CrosLibrary::Get()->GetNetworkLibrary()->
+      LoadOncNetworks(onc_blob, passcode);
   SendJavascriptCommand("receivedONCFileParse",
-                        Value::CreateStringValue(error));
+                        Value::CreateBooleanValue(success));
 }
 #endif