diff options
author | lfg <lfg@chromium.org> | 2015-03-19 09:49:53 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-03-19 16:50:36 +0000 |
commit | 052af418cf20a22c485825cd4e2628c4925b5e64 (patch) | |
tree | 4e0d30b15a52b44d5d999493d071030a168bff60 | |
parent | 8824ed5b8ecc1eee3053aee6c80f77a31c32c11f (diff) | |
download | chromium_src-052af418cf20a22c485825cd4e2628c4925b5e64.zip chromium_src-052af418cf20a22c485825cd4e2628c4925b5e64.tar.gz chromium_src-052af418cf20a22c485825cd4e2628c4925b5e64.tar.bz2 |
Revert of Add RoutingIDIssuer to help track routing id issues. (patchset #4 id:60001 of https://codereview.chromium.org/999193003/)
Reason for revert:
We've already collected enough data to triage this bug.
Original issue's description:
> This is a re-land of https://codereview.chromium.org/643733002.
>
> This CL adds a RoutingIDIssuer that verifies that the routing IDs are being sent to the correct process.
>
> BUG=464633
> TBR=sky@chromium.org
>
> Committed: https://crrev.com/5ae65efdfd51de794fb544195fad271dd90ff0b1
> Cr-Commit-Position: refs/heads/master@{#320207}
TBR=nasko@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=464633
Review URL: https://codereview.chromium.org/1019913002
Cr-Commit-Position: refs/heads/master@{#321380}
16 files changed, 4 insertions, 223 deletions
diff --git a/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc b/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc index c3e869d..9e7d00d 100644 --- a/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc +++ b/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc @@ -21,7 +21,6 @@ #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/ui_test_utils.h" #include "content/public/renderer/render_view.h" -#include "content/public/test/routing_id_mangling_disabler.h" #include "crypto/sha2.h" #include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" @@ -200,7 +199,6 @@ class PhishingClassifierTest : public InProcessBrowserTest { scoped_ptr<Scorer> scorer_; scoped_ptr<PhishingClassifier> classifier_; MockFeatureExtractorClock* clock_; // Owned by classifier_. - content::RoutingIDManglingDisabler mangling_disabler_; // Features that are in the model. const std::string url_tld_token_net_; diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc index 2623d5d..8f7f29884 100644 --- a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc +++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc @@ -26,7 +26,6 @@ #include "content/public/browser/web_contents.h" #include "content/public/renderer/render_view.h" #include "content/public/test/browser_test_utils.h" -#include "content/public/test/routing_id_mangling_disabler.h" #include "content/public/test/test_navigation_observer.h" #include "content/public/test/test_utils.h" #include "net/dns/mock_host_resolver.h" @@ -264,7 +263,6 @@ class PhishingClassifierDelegateTest : public InProcessBrowserTest { StrictMock<MockPhishingClassifier>* classifier_; // Owned by |delegate_|. PhishingClassifierDelegate* delegate_; // Owned by the RenderView. scoped_refptr<content::MessageLoopRunner> runner_; - content::RoutingIDManglingDisabler mangling_disabler_; }; IN_PROC_BROWSER_TEST_F(PhishingClassifierDelegateTest, Navigation) { diff --git a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc index 6392c90..3d1814f 100644 --- a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc +++ b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc @@ -29,7 +29,6 @@ #include "content/public/browser/web_contents.h" #include "content/public/renderer/render_view.h" #include "content/public/test/browser_test_utils.h" -#include "content/public/test/routing_id_mangling_disabler.h" #include "content/public/test/test_utils.h" #include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" @@ -180,7 +179,6 @@ class PhishingDOMFeatureExtractorTest : public InProcessBrowserTest { // Any urls not in this map are served a 404 error. std::map<std::string, std::string> responses_; - content::RoutingIDManglingDisabler mangling_disabler_; scoped_ptr<net::test_server::EmbeddedTestServer> embedded_test_server_; MockFeatureExtractorClock clock_; scoped_ptr<PhishingDOMFeatureExtractor> extractor_; diff --git a/content/browser/accessibility/site_per_process_accessibility_browsertest.cc b/content/browser/accessibility/site_per_process_accessibility_browsertest.cc index 3fd284d..bf9c189 100644 --- a/content/browser/accessibility/site_per_process_accessibility_browsertest.cc +++ b/content/browser/accessibility/site_per_process_accessibility_browsertest.cc @@ -23,7 +23,6 @@ #include "content/public/test/browser_test_utils.h" #include "content/public/test/content_browser_test.h" #include "content/public/test/content_browser_test_utils.h" -#include "content/public/test/routing_id_mangling_disabler.h" #include "content/public/test/test_utils.h" #include "content/shell/browser/shell.h" #include "content/test/accessibility_browser_test_utils.h" @@ -38,11 +37,6 @@ class SitePerProcessAccessibilityBrowserTest : public SitePerProcessBrowserTest { public: SitePerProcessAccessibilityBrowserTest() {} - - // TODO(morrita): This is fishy. This test shouldn't rely on the - // abasence of routing_id mangling. Something seems wrong. - // See http://crbug.com/422136 for more updates. - content::RoutingIDManglingDisabler mangling_disabler_; }; // Utility function to determine if an accessibility tree has finished loading diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc index 7ada89c..4152ce2 100644 --- a/content/browser/renderer_host/render_process_host_impl.cc +++ b/content/browser/renderer_host/render_process_host_impl.cc @@ -1017,8 +1017,6 @@ void RenderProcessHostImpl::AddRoute( IPC::Listener* listener) { CHECK(!listeners_.Lookup(routing_id)) << "Found Routing ID Conflict: " << routing_id; - CHECK(widget_helper_->IsRoutingIDProbablyValid(routing_id)) - << "Found Routing ID conflicts: " << routing_id; listeners_.AddWithID(listener, routing_id); } diff --git a/content/browser/renderer_host/render_widget_helper.cc b/content/browser/renderer_host/render_widget_helper.cc index eceb407..e9d83f4 100644 --- a/content/browser/renderer_host/render_widget_helper.cc +++ b/content/browser/renderer_host/render_widget_helper.cc @@ -16,7 +16,6 @@ #include "content/browser/loader/resource_dispatcher_host_impl.h" #include "content/browser/renderer_host/render_process_host_impl.h" #include "content/browser/renderer_host/render_view_host_impl.h" -#include "content/browser/renderer_host/routing_id_issuer.h" #include "content/common/view_messages.h" namespace content { @@ -38,7 +37,6 @@ void AddWidgetHelper(int render_process_id, RenderWidgetHelper::RenderWidgetHelper() : render_process_id_(-1), - routing_id_issuer_(RoutingIDIssuer::Create()), resource_dispatcher_host_(NULL) { } @@ -65,11 +63,7 @@ void RenderWidgetHelper::Init( } int RenderWidgetHelper::GetNextRoutingID() { - return routing_id_issuer_->IssueNext(); -} - -bool RenderWidgetHelper::IsRoutingIDProbablyValid(int routing_id) const { - return routing_id_issuer_->IsProbablyValid(routing_id); + return next_routing_id_.GetNext() + 1; } // static diff --git a/content/browser/renderer_host/render_widget_helper.h b/content/browser/renderer_host/render_widget_helper.h index e8d7921..7f32035 100644 --- a/content/browser/renderer_host/render_widget_helper.h +++ b/content/browser/renderer_host/render_widget_helper.h @@ -7,9 +7,9 @@ #include <map> +#include "base/atomic_sequence_num.h" #include "base/containers/hash_tables.h" #include "base/memory/ref_counted.h" -#include "base/memory/scoped_ptr.h" #include "base/process/process.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/content_browser_client.h" @@ -32,7 +32,6 @@ struct ViewMsg_SwapOut_Params; namespace content { class GpuProcessHost; class ResourceDispatcherHostImpl; -class RoutingIDIssuer; class SessionStorageNamespace; // Instantiated per RenderProcessHost to provide various optimizations on @@ -80,9 +79,6 @@ class RenderWidgetHelper // Gets the next available routing id. This is thread safe. int GetNextRoutingID(); - // Probablistically verify if the ID is issued by this helper. - // Thread safe. - bool IsRoutingIDProbablyValid(int routing_id) const; // IO THREAD ONLY ----------------------------------------------------------- @@ -154,7 +150,8 @@ class RenderWidgetHelper int render_process_id_; - scoped_ptr<RoutingIDIssuer> routing_id_issuer_; + // The next routing id to use. + base::AtomicSequenceNumber next_routing_id_; ResourceDispatcherHostImpl* resource_dispatcher_host_; diff --git a/content/browser/renderer_host/routing_id_issuer.cc b/content/browser/renderer_host/routing_id_issuer.cc deleted file mode 100644 index dbcb7f9..0000000 --- a/content/browser/renderer_host/routing_id_issuer.cc +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "content/browser/renderer_host/routing_id_issuer.h" - -namespace content { - -namespace { - -const int kManglerBits = 6; -const int kIDBits = 31 - kManglerBits; -const int kManglerWrap = 1 << kManglerBits; -const int kIDWrap = 1 << kIDBits; -const int kIDMask = kIDWrap - 1; - -static base::StaticAtomicSequenceNumber g_manglerSequence; -static int g_disablingIDManglingCount = 0; - -int GetNextMangler() { - // Does +1 to always gives some mangler. - return (g_manglerSequence.GetNext() % (kManglerWrap - 1)) + 1; -} - -} // namespace - -scoped_ptr<RoutingIDIssuer> RoutingIDIssuer::Create() { - if (0 < g_disablingIDManglingCount) - return make_scoped_ptr(new RoutingIDIssuer(0)); - - DCHECK(0 == g_disablingIDManglingCount); - return make_scoped_ptr(new RoutingIDIssuer(GetNextMangler())); -} - -scoped_ptr<RoutingIDIssuer> RoutingIDIssuer::CreateWithMangler(int mangler) { - return make_scoped_ptr(new RoutingIDIssuer(mangler)); -} - -RoutingIDIssuer::RoutingIDIssuer(int mangler) : mangler_(mangler) { - DCHECK(0 <= mangler_ && mangler_ < kManglerWrap) << mangler_ - << ">=" << kManglerWrap; -} - -int RoutingIDIssuer::IssueNext() { - return (mangler_ << kIDBits) | ((next_.GetNext() + 1) & kIDMask); -} - -bool RoutingIDIssuer::IsProbablyValid(int issued_id) const { - return (mangler_ << kIDBits) == (issued_id & ~kIDMask); -} - -void RoutingIDIssuer::DisableIDMangling() { - g_disablingIDManglingCount++; -} - -void RoutingIDIssuer::EnableIDMangling() { - g_disablingIDManglingCount--; - DCHECK(0 <= g_disablingIDManglingCount); -} - -} // namespace content diff --git a/content/browser/renderer_host/routing_id_issuer.h b/content/browser/renderer_host/routing_id_issuer.h deleted file mode 100644 index 36fc452..0000000 --- a/content/browser/renderer_host/routing_id_issuer.h +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CONTENT_BROWSER_RENDERER_HOST_ROUTING_ID_ISSUER_H_ -#define CONTENT_BROWSER_RENDERER_HOST_ROUTING_ID_ISSUER_H_ - -#include "base/atomic_sequence_num.h" -#include "base/macros.h" -#include "base/memory/scoped_ptr.h" -#include "base/synchronization/lock.h" -#include "content/common/content_export.h" -#include "ipc/ipc_message.h" - -namespace content { - -// A debug facility for tightening the routing ID usage across multiple -// RenderProcessHost. The tracked ID is used by RenderProcessHost -// to verify if the given ID is correct. -class CONTENT_EXPORT RoutingIDIssuer { - public: - static scoped_ptr<RoutingIDIssuer> Create(); - static scoped_ptr<RoutingIDIssuer> CreateWithMangler(int mangler); - - int IssueNext(); - bool IsProbablyValid(int issued_id) const; - - private: - friend class RoutingIDManglingDisabler; - - static void DisableIDMangling(); - static void EnableIDMangling(); - - explicit RoutingIDIssuer(int mangler); - - base::AtomicSequenceNumber next_; - const int mangler_; - - DISALLOW_COPY_AND_ASSIGN(RoutingIDIssuer); -}; - -} // namespace content - -#endif // CONTENT_BROWSER_RENDERER_HOST_ROUTING_ID_ISSUER_H_ diff --git a/content/browser/renderer_host/routing_id_issuer_unittest.cc b/content/browser/renderer_host/routing_id_issuer_unittest.cc deleted file mode 100644 index da1e528..0000000 --- a/content/browser/renderer_host/routing_id_issuer_unittest.cc +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "content/browser/renderer_host/routing_id_issuer.h" - -#include "testing/gtest/include/gtest/gtest.h" - -namespace content { -namespace { - -const int kMangler = 5; - -class RoutingIDIssuerTest : public testing::Test {}; - -TEST_F(RoutingIDIssuerTest, IssueNext) { - scoped_ptr<RoutingIDIssuer> target = - RoutingIDIssuer::CreateWithMangler(kMangler); - int id0 = target->IssueNext(); - int id1 = target->IssueNext(); - EXPECT_EQ(id0 + 1, id1); -} - -TEST_F(RoutingIDIssuerTest, IsProbablyValid) { - scoped_ptr<RoutingIDIssuer> proper = - RoutingIDIssuer::CreateWithMangler(kMangler); - scoped_ptr<RoutingIDIssuer> foreign = - RoutingIDIssuer::CreateWithMangler(kMangler + 1); - - EXPECT_FALSE(proper->IsProbablyValid(kMangler)); - - int proper_id = proper->IssueNext(); - int foreign_id = foreign->IssueNext(); - EXPECT_TRUE(proper->IsProbablyValid(proper_id)); - EXPECT_TRUE(foreign->IsProbablyValid(foreign_id)); - EXPECT_FALSE(proper->IsProbablyValid(foreign_id)); - EXPECT_FALSE(foreign->IsProbablyValid(proper_id)); -} - -} // namespace -} // namespace contnet diff --git a/content/content_browser.gypi b/content/content_browser.gypi index d213d74..1cd29e3 100644 --- a/content/content_browser.gypi +++ b/content/content_browser.gypi @@ -1218,8 +1218,6 @@ 'browser/renderer_host/render_widget_resize_helper.h', 'browser/renderer_host/renderer_frame_manager.cc', 'browser/renderer_host/renderer_frame_manager.h', - 'browser/renderer_host/routing_id_issuer.cc', - 'browser/renderer_host/routing_id_issuer.h', 'browser/renderer_host/sandbox_ipc_linux.cc', 'browser/renderer_host/sandbox_ipc_linux.h', 'browser/renderer_host/text_input_client_mac.h', diff --git a/content/content_tests.gypi b/content/content_tests.gypi index a0a9b4f..f9328bd 100644 --- a/content/content_tests.gypi +++ b/content/content_tests.gypi @@ -67,8 +67,6 @@ 'public/test/render_view_test.h', 'public/test/render_widget_test.cc', 'public/test/render_widget_test.h', - 'public/test/routing_id_mangling_disabler.cc', - 'public/test/routing_id_mangling_disabler.h', 'public/test/sandbox_file_system_test_helper.cc', 'public/test/sandbox_file_system_test_helper.h', 'public/test/test_browser_context.cc', @@ -523,7 +521,6 @@ 'browser/renderer_host/render_widget_host_view_base_unittest.cc', 'browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm', 'browser/renderer_host/render_widget_host_view_mac_unittest.mm', - 'browser/renderer_host/routing_id_issuer_unittest.cc', 'browser/renderer_host/text_input_client_mac_unittest.mm', 'browser/renderer_host/web_input_event_aura_unittest.cc', 'browser/renderer_host/websocket_dispatcher_host_unittest.cc', diff --git a/content/public/test/routing_id_mangling_disabler.cc b/content/public/test/routing_id_mangling_disabler.cc deleted file mode 100644 index b1e83a1..0000000 --- a/content/public/test/routing_id_mangling_disabler.cc +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "content/public/test/routing_id_mangling_disabler.h" - -#include "content/browser/renderer_host/routing_id_issuer.h" - -namespace content { - -RoutingIDManglingDisabler::RoutingIDManglingDisabler() { - RoutingIDIssuer::DisableIDMangling(); -} - -RoutingIDManglingDisabler::~RoutingIDManglingDisabler() { - RoutingIDIssuer::EnableIDMangling(); -} - -} // namespace content diff --git a/content/public/test/routing_id_mangling_disabler.h b/content/public/test/routing_id_mangling_disabler.h deleted file mode 100644 index 5c23174..0000000 --- a/content/public/test/routing_id_mangling_disabler.h +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CONTENT_PUBLIC_TEST_ROUTIND_ID_MANGLING_DISABLER_H_ -#define CONTENT_PUBLIC_TEST_ROUTIND_ID_MANGLING_DISABLER_H_ - -namespace content { - -// Some tests unfortunately assume that the routing ID starts from -// one. This class temporarily satisfies such an assumption. -class RoutingIDManglingDisabler { - public: - RoutingIDManglingDisabler(); - ~RoutingIDManglingDisabler(); -}; - -} // namespace content - -#endif // CONTENT_PUBLIC_TEST_ROUTIND_ID_MANGLING_DISABLER_H_ diff --git a/content/renderer/dom_serializer_browsertest.cc b/content/renderer/dom_serializer_browsertest.cc index 45dac5b..b175942 100644 --- a/content/renderer/dom_serializer_browsertest.cc +++ b/content/renderer/dom_serializer_browsertest.cc @@ -15,7 +15,6 @@ #include "content/public/renderer/render_view_observer.h" #include "content/public/test/content_browser_test.h" #include "content/public/test/content_browser_test_utils.h" -#include "content/public/test/routing_id_mangling_disabler.h" #include "content/public/test/test_utils.h" #include "content/renderer/savable_resources.h" #include "content/shell/browser/shell.h" @@ -775,8 +774,6 @@ class DomSerializerTests : public ContentBrowserTest, // The local_directory_name_ is dummy relative path of directory which // contain all saved auxiliary files included all sub frames and resources. const base::FilePath local_directory_name_; - - content::RoutingIDManglingDisabler mangling_disabler_; }; // If original contents have document type, the serialized contents also have diff --git a/content/renderer/resource_fetcher_browsertest.cc b/content/renderer/resource_fetcher_browsertest.cc index fee84ee..92a57a6 100644 --- a/content/renderer/resource_fetcher_browsertest.cc +++ b/content/renderer/resource_fetcher_browsertest.cc @@ -15,7 +15,6 @@ #include "content/public/renderer/render_view.h" #include "content/public/test/content_browser_test.h" #include "content/public/test/content_browser_test_utils.h" -#include "content/public/test/routing_id_mangling_disabler.h" #include "content/public/test/test_utils.h" #include "content/shell/browser/shell.h" #include "third_party/WebKit/public/platform/WebURLResponse.h" @@ -285,8 +284,6 @@ class ResourceFetcherTests : public ContentBrowserTest { EXPECT_EQ(delegate->response().httpStatusCode(), 200); EXPECT_EQ(kHeader, delegate->data()); } - - content::RoutingIDManglingDisabler routing_id_mangling_disabler_; }; // Test a fetch from the test server. |