Issue metadata
Sign in to add a comment
|
Make HttpServerPropertiesManager better support its use in the NetworkService. |
||||||||||||||||||||||
Issue descriptionHttpServerPropertiesManager was designed to write to prefs on one thread, and be used by the network stack on another. As we set up the network service, these threads have become the same thread. Also, prefs loaded from disk are no longer available when the HttpServerPropertiesManager is created. We need to improve HttpServerPropertiesManager to better support both these cases. Ideally, we can completely remove support for the old case, once iOS also uses the network service. In particular, we should: * Flush final state to disk on destruction. Since we can write to prefs on destruction when there's a single thread, this means we only lose state on unclear shutdowns. * Remove IntializeOnNetworkSequence/ShutdownOnPrefSequence (Or at least make them no-ops when the sequences are the same). * Wait for a single the pref file is readable before reading initial prefs. The current behavior here may be broken in Cronet currently, if we don't delay creation until the prefs are loaded. While the changes themselves are fairly simple, the test infrastructure for HttpServerPropertiesManager is a bit of a beast, so suspect getting things working reasonable there will be a bit of a challenge. I'd advocate for complete rewriting the tests to no longer subclass HttpServerProperties, so we're testing just the interface, rather than the implementation. Also might be good to move away from Gmock.
,
Sep 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f077446d3f9ce9f634efbcaf6d760b0cc3bc1e6a commit f077446d3f9ce9f634efbcaf6d760b0cc3bc1e6a Author: Matt Menke <mmenke@chromium.org> Date: Fri Sep 29 15:15:23 2017 ios: Make PrefServiceAdapter use the network pref store. This means that it only lives on on thread: The network thread, instead of jumping between the UI thread and the IO thread, since the network pref store lives on the IO thread, just like the network stack. A similar change was made to chrome/, as part of network servicification (Though in Chrome it's created by the NetworkService now). In a followup CL, I'll simplify HttpServerPropertiesManager to only support the case where the prefs thread is the network thread. Bug: 768884 Change-Id: I92c97e8e5fb3f5cab46f1c998b92eaec48a3db5f Reviewed-on: https://chromium-review.googlesource.com/687803 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: David Roger <droger@chromium.org> Cr-Commit-Position: refs/heads/master@{#505375} [modify] https://crrev.com/f077446d3f9ce9f634efbcaf6d760b0cc3bc1e6a/ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm [modify] https://crrev.com/f077446d3f9ce9f634efbcaf6d760b0cc3bc1e6a/ios/chrome/browser/net/http_server_properties_manager_factory.cc [modify] https://crrev.com/f077446d3f9ce9f634efbcaf6d760b0cc3bc1e6a/ios/chrome/browser/net/http_server_properties_manager_factory.h [modify] https://crrev.com/f077446d3f9ce9f634efbcaf6d760b0cc3bc1e6a/ios/chrome/browser/prefs/browser_prefs.mm
,
Sep 29 2017
,
Oct 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/258cfcbaad2c6865482282f349c260fe5ed91792 commit 258cfcbaad2c6865482282f349c260fe5ed91792 Author: Matt Menke <mmenke@chromium.org> Date: Thu Oct 05 22:54:32 2017 HttpServerPropertiesManager: Remove use of two threads. All consumers now use a single thread for prefs and the network stack, so there's no need for it to have thread hopping logic. This also makes HttpServerPropertiesManager better support the case where prefs aren't loaded when it's created (Always the case in consumers), and makes it flush to prefs on destruction, which can now be done, since the net and prefs threads are the same. This also fixes a crasher in the single-thread case, due to the use of base::Unretained when calling from the prefs thread to the network thread, which doesn't work when they're the same thread. Bug: 768884 , 770179 NOPRESUBMIT=true (Unfortunately, the tests use banned APIs. This use predates this CL, and this CL doesn't introduce new uses of them. Fixing a crasher is more important than removing banned APIs). Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I2692e3c02ff6028b585416808b60cabcf9d2e2e6 Reviewed-on: https://chromium-review.googlesource.com/693054 Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org> Reviewed-by: David Roger <droger@chromium.org> Reviewed-by: Bence Béky <bnc@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#506897} [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/components/cronet/android/cronet_url_request_context_adapter.h [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/components/cronet/cronet_prefs_manager.cc [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/components/cronet/ios/cronet_environment.mm [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/content/network/http_server_properties_pref_delegate.cc [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/content/network/http_server_properties_pref_delegate.h [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/content/network/network_context.cc [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/content/network/network_context.h [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/content/network/network_context_unittest.cc [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.h [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/ios/chrome/browser/net/http_server_properties_manager_factory.cc [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/ios/chrome/browser/net/http_server_properties_manager_factory.h [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/net/http/http_server_properties_manager.cc [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/net/http/http_server_properties_manager.h [modify] https://crrev.com/258cfcbaad2c6865482282f349c260fe5ed91792/net/http/http_server_properties_manager_unittest.cc
,
Oct 6 2017
,
Nov 7 2017
Apologies, applied the wrong component in bulk. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by mmenke@chromium.org
, Sep 26 2017