NetworkService requires MetricsService when downloading without full browser |
||||||||
Issue descriptionAbort message: '[FATAL:network_service_instance.cc(198)] Check failed: BrowserThread::CurrentlyOn(BrowserThread::UI). ' v------> base::debug::(anonymous namespace)::DebugBreak() /usr/local/google/code/clankium/src/base/debug/debugger_posix.cc:229:5 0013cbfd base::debug::BreakDebugger() /usr/local/google/code/clankium/src/base/debug/debugger_posix.cc:263:0 000dcfab logging::LogMessage::~LogMessage() /usr/local/google/code/clankium/src/base/logging.cc:876:7 00d29031 content::GetNetworkConnectionTracker() /usr/local/google/code/clankium/src/content/browser/network_service_instance.cc:198:3 00d5e7c5 data_use_measurement::ChromeDataUseMeasurement::GetInstance() /usr/local/google/code/clankium/src/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc:74:27 009fdfc5 ChromeContentBrowserClient::OnNetworkServiceDataUseUpdate(int, long long, long long) /usr/local/google/code/clankium/src/chrome/browser/chrome_content_browser_client.cc:4984:7 008ecde7 network::mojom::NetworkServiceClientStubDispatch::Accept(network::mojom::NetworkServiceClient*, mojo::Message*) /usr/local/google/code/clankium/src/out/Default/gen/services/network/public/mojom/network_service.mojom.cc:3775:13 00016b33 mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:423:32 000167cb mojo::FilterChain::Accept(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/filter_chain.cc:40:17 000174bd mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:306:19 [...] Working around the first DCHECK exposes a second one: v------> base::debug::(anonymous namespace)::DebugBreak() /usr/local/google/code/clankium/src/base/debug/debugger_posix.cc:229:5 0013cbfd base::debug::BreakDebugger() /usr/local/google/code/clankium/src/base/debug/debugger_posix.cc:263:0 000dcfab logging::LogMessage::~LogMessage() /usr/local/google/code/clankium/src/base/logging.cc:876:7 00a0c14f data_use_measurement::(anonymous namespace)::UpdateMetricsUsagePrefs(long long, bool, bool) /usr/local/google/code/clankium/src/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc:37:3 00d2a0ad data_use_measurement::ChromeDataUseMeasurement::ReportNetworkServiceDataUse(int, long long, long long) /usr/local/google/code/clankium/src/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc:116:3 008ecde7 network::mojom::NetworkServiceClientStubDispatch::Accept(network::mojom::NetworkServiceClient*, mojo::Message /usr/local/google/code/clankium/src/out/Default/gen/services/network/public/mojom/network_service.mojom.cc:3775:13 00016b33 mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:423:32 000167cb mojo::FilterChain::Accept(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/filter_chain.cc:40:17 000174bd mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:306:19 [...] Removing the second DCHECK exposes a crash on the following line: auto* metrics_service = g_browser_process->metrics_service(); Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0 in tid 21312 (oid.apps.chrome), pid 21312 (oid.apps.chrome) 00a0c116 data_use_measurement::(anonymous namespace)::UpdateMetricsUsagePrefs(long long, bool, bool) /usr/local/google/code/clankium/src/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc:40:46 00d2a065 data_use_measurement::ChromeDataUseMeasurement::ReportNetworkServiceDataUse(int, long long, long long) /usr/local/google/code/clankium/src/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc:116:3 008ecde7 network::mojom::NetworkServiceClientStubDispatch::Accept(network::mojom::NetworkServiceClient*, mojo::Message*) /usr/local/google/code/clankium/src/out/Default/gen/services/network/public/mojom/network_service.mojom.cc:3775:13 00016b33 mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:423:32 000167cb mojo::FilterChain::Accept(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/filter_chain.cc:40:17 000174bd mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message*) /usr/local/google/code/clankium/src/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:306:19 [...] g_browser_process is not set at that point, since we're in service manager-only mode. The MetricsService is not instantiated either, though MetricsServiceManager, which could create it, is.
,
Nov 7
,
Nov 8
So the crux of the matter is whether we should be starting MetricsService early along with NetworkService, since there is this dependency chain between them that goes through ChromeDataUseMeasurement. If it's fine to start MetricsService early too, we should do so and find a suitable place to hold it until the browser process takes ownership. If not, we should have ChromeDataUseMeasurement sandbag the UpdateMetricsUsagePrefs calls somewhere until the MetricsService exists and can be sent the data. Alexei, what do you think is the correct route? For context, this is part of the effort to fix crbug.com/729596
,
Nov 8
When you say "start MetricsService" what do you mean exactly? It has different states. For example, the object being created, InitializeMetricsRecordingState(), Start() and so forth. I think if it's just the object being created, that should be fine. If it's the other ones, we'd have to reason about the specifics.
,
Nov 8
For this particular DataUseTracker::UpdateUsagePref() codepath, creating the MetricsService object is enough. Alternatively, if(g_browser_process) could be added and the failing DCHECKS can be converted to use ThreadChecker in chrome_data_use_measurement.cc.
,
Nov 9
Considering #5, it seems to me that creating the Metrics service is the correct option in the long run. As for #6, if we fix the DCHECKS and check "if (g_browser_process)", what would we do if it is not there? If we bail out, the logs are lost, I assume. hanxi@ suggested storing the call parameters until g_browser_process exists and we can flush whatever was stored, but that adds some complexity and another layer of caching.
,
Nov 9
Actually, to avoid creating Metrics and Notifications, it might be better to delay the logs. Hopefully this is the only edge between NetworkService and MetricsService to handle. If not, we'll reevaluate.
,
Nov 9
,
Nov 9
rajendrant@, there is already a check inside UpdateMetricsUsagePrefs that can cause the logs to not be sent:
if (metrics_service) {
metrics_service->UpdateMetricsUsagePrefs(
base::saturated_cast<int>(total_bytes), is_cellular,
is_metrics_service_usage);
}
Is the (!metrics_service) case expected to happen? That is, is it ok to ignore these logs in some cases?
,
Nov 9
My initial impression was that it was failing in some testcase or so. But it seems to be a special case that does not spawn full browser. cc gayane@ who implemented the UpdateMetricsUsagePrefs. Re #c10, I do not see (!metrics_service) being hit. The first call to BrowserProcessImpl::GetMetricsServicesManager() creates MetricsServicesManager, ChromeMetricsServicesManagerClient, MetricsService, MetricsReportingService, DataUseTracker are created one by one with the exception that DataUseTracker is created only on android. Gayane/Alexei, Since DataUseTracker::UpdateMetricsUsagePrefs() finally ends up storing some prefs, can this be flattened out to a static function that uses g_browser_process->local_state() to save the prefs, without creating these objects.
,
Nov 12
I think it is a good idea to have a static function which only update a given |local_state|. Gayane/Alexei, WDYT?
,
Nov 12
https://chromium-review.googlesource.com/c/chromium/src/+/1332155 Working on adapting the tests.
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a061c829b65ad5865f68112aac182c89cc622311 commit a061c829b65ad5865f68112aac182c89cc622311 Author: Henrique Nakashima <hnakashima@chromium.org> Date: Thu Nov 15 23:09:44 2018 Flatten UpdateMetricsUsagePrefs - skip Metrics and Reporting service. UpdateMetricsUsagePrefs() is a chain of calls: - ChromeDataUseMeasurement::UpdateDataUseToMetricsService() -> UpdateMetricsUsagePrefs() -> MetricsService::UpdateMetricsUsagePrefs() -> ReportingService::UpdateMetricsUsagePrefs() -> DataUseTracker::UpdateMetricsUsagePrefs() It can be reduced to: - ChromeDataUseMeasurement::UpdateDataUseToMetricsService() -> UpdateMetricsUsagePrefs() -> DataUseTracker::UpdateMetricsUsagePrefs() This removes the dependency from ChromeDataUseMeasurement to MetricsService and ReportingService. Bug: 902791 Change-Id: I38a6d22d1ff823134c79bc342ea32be0f89cda77 Reviewed-on: https://chromium-review.googlesource.com/c/1334267 Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: rajendrant <rajendrant@chromium.org> Cr-Commit-Position: refs/heads/master@{#608567} [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/components/metrics/data_use_tracker.cc [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/components/metrics/data_use_tracker.h [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/components/metrics/data_use_tracker_unittest.cc [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/components/metrics/metrics_service.cc [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/components/metrics/metrics_service.h [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/components/metrics/reporting_service.cc [modify] https://crrev.com/a061c829b65ad5865f68112aac182c89cc622311/components/metrics/reporting_service.h
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b9de7c933418467a7c47a6d9d16c32249230e50 commit 4b9de7c933418467a7c47a6d9d16c32249230e50 Author: Henrique Nakashima <hnakashima@chromium.org> Date: Wed Nov 21 00:15:22 2018 Revert "Flatten UpdateMetricsUsagePrefs - skip Metrics and Reporting service." This reverts commit a061c829b65ad5865f68112aac182c89cc622311. Reason for revert: Suspect in crbug.com/906242 Original change's description: > Flatten UpdateMetricsUsagePrefs - skip Metrics and Reporting service. > > UpdateMetricsUsagePrefs() is a chain of calls: > - ChromeDataUseMeasurement::UpdateDataUseToMetricsService() > -> UpdateMetricsUsagePrefs() > -> MetricsService::UpdateMetricsUsagePrefs() > -> ReportingService::UpdateMetricsUsagePrefs() > -> DataUseTracker::UpdateMetricsUsagePrefs() > > It can be reduced to: > - ChromeDataUseMeasurement::UpdateDataUseToMetricsService() > -> UpdateMetricsUsagePrefs() > -> DataUseTracker::UpdateMetricsUsagePrefs() > > This removes the dependency from ChromeDataUseMeasurement to > MetricsService and ReportingService. > > Bug: 902791 > Change-Id: I38a6d22d1ff823134c79bc342ea32be0f89cda77 > Reviewed-on: https://chromium-review.googlesource.com/c/1334267 > Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: rajendrant <rajendrant@chromium.org> > Cr-Commit-Position: refs/heads/master@{#608567} TBR=asvitkine@chromium.org,bcwhite@chromium.org,rajendrant@chromium.org,hnakashima@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 902791 Change-Id: Iad1f49a8bae42e60ae5a9c68509d4cb38db683ef Reviewed-on: https://chromium-review.googlesource.com/c/1344817 Reviewed-by: Henrique Nakashima <hnakashima@chromium.org> Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> Cr-Commit-Position: refs/heads/master@{#609861} [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/components/metrics/data_use_tracker.cc [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/components/metrics/data_use_tracker.h [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/components/metrics/data_use_tracker_unittest.cc [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/components/metrics/metrics_service.cc [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/components/metrics/metrics_service.h [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/components/metrics/reporting_service.cc [modify] https://crrev.com/4b9de7c933418467a7c47a6d9d16c32249230e50/components/metrics/reporting_service.h
,
Nov 26
Changing owner since hnakashima@ is actively working on it.
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ff7197996b04730cfbaa5801a57ef174a56ee3a commit 0ff7197996b04730cfbaa5801a57ef174a56ee3a Author: Henrique Nakashima <hnakashima@chromium.org> Date: Tue Nov 27 19:29:57 2018 Reland "Flatten UpdateMetricsUsagePrefs - skip Metrics and Reporting service." This is a reland of a061c829b65ad5865f68112aac182c89cc622311 Reverted due to being a suspect in crbug.com/906242, but after reverting the bug persists and was bisected to another CL. Original change's description: > Flatten UpdateMetricsUsagePrefs - skip Metrics and Reporting service. > > UpdateMetricsUsagePrefs() is a chain of calls: > - ChromeDataUseMeasurement::UpdateDataUseToMetricsService() > -> UpdateMetricsUsagePrefs() > -> MetricsService::UpdateMetricsUsagePrefs() > -> ReportingService::UpdateMetricsUsagePrefs() > -> DataUseTracker::UpdateMetricsUsagePrefs() > > It can be reduced to: > - ChromeDataUseMeasurement::UpdateDataUseToMetricsService() > -> UpdateMetricsUsagePrefs() > -> DataUseTracker::UpdateMetricsUsagePrefs() > > This removes the dependency from ChromeDataUseMeasurement to > MetricsService and ReportingService. > > Bug: 902791 > Change-Id: I38a6d22d1ff823134c79bc342ea32be0f89cda77 > Reviewed-on: https://chromium-review.googlesource.com/c/1334267 > Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: rajendrant <rajendrant@chromium.org> > Cr-Commit-Position: refs/heads/master@{#608567} Bug: 902791 Change-Id: I27abaac1d265e63ae5a921ab2839a77ae8582720 Reviewed-on: https://chromium-review.googlesource.com/c/1348249 Reviewed-by: rajendrant <rajendrant@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> Cr-Commit-Position: refs/heads/master@{#611250} [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/components/metrics/data_use_tracker.cc [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/components/metrics/data_use_tracker.h [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/components/metrics/data_use_tracker_unittest.cc [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/components/metrics/metrics_service.cc [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/components/metrics/metrics_service.h [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/components/metrics/reporting_service.cc [modify] https://crrev.com/0ff7197996b04730cfbaa5801a57ef174a56ee3a/components/metrics/reporting_service.h
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83 commit 8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83 Author: Henrique Nakashima <hnakashima@chromium.org> Date: Thu Nov 29 23:01:53 2018 Pipe local_state into ChromeDataUseMeasurement. This removes the dependency ChromeDataUseMeasurement has on g_browser_process (when kNetworkService is enabled) and fixes a crash when starting in service-manager-only mode. Bug: 902791 Change-Id: I0ed11f4735f4494247a7c17767b53aab695d0f14 Reviewed-on: https://chromium-review.googlesource.com/c/1334334 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Min Qin <qinmin@chromium.org> Reviewed-by: rajendrant <rajendrant@chromium.org> Commit-Queue: Henrique Nakashima <hnakashima@chromium.org> Cr-Commit-Position: refs/heads/master@{#612421} [modify] https://crrev.com/8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83/chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc [modify] https://crrev.com/8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83/chrome/browser/data_use_measurement/chrome_data_use_measurement.cc [modify] https://crrev.com/8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83/chrome/browser/data_use_measurement/chrome_data_use_measurement.h [modify] https://crrev.com/8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.cc [modify] https://crrev.com/8941aad9380b5e2fb70f1cc7f3ebc9cfef359b83/content/browser/network_service_instance.cc
,
Nov 30
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 Deleted