New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 707836 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , iOS
Pri: 2
Type: Bug



Sign in to add a comment

StatisticsRecorder::Initialize() is called too late

Project Member Reported by xunji...@chromium.org, Apr 3 2017

Issue description

https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/70121e5f-c464-4d21-943e-323ed7f3c8f8%40chromium.org?utm_medium=email&utm_source=footer

According to the chromium-dev@ thread above, StatisticsRecorder::Initialize() is called too late in Cronet. This causes leakage of histograms logged before GetHistogramDeltas().


 
Owner: xunji...@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e64a797f7b6b4fb54536bbbc4c809c4fc6da3e1

commit 7e64a797f7b6b4fb54536bbbc4c809c4fc6da3e1
Author: xunjieli <xunjieli@chromium.org>
Date: Wed Apr 05 19:43:26 2017

[Cronet] Initialize StatisticsRecorder early

StatisticsRecorder must be initialized before histograms are emitted. Otherwise,
StatisticsRecorder::FactoryGet() leaks one histogram per call for a given name.

BUG= 707836 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2790063004
Cr-Commit-Position: refs/heads/master@{#462185}

[modify] https://crrev.com/7e64a797f7b6b4fb54536bbbc4c809c4fc6da3e1/components/cronet/android/cronet_library_loader.cc
[modify] https://crrev.com/7e64a797f7b6b4fb54536bbbc4c809c4fc6da3e1/components/cronet/android/cronet_url_request_context_adapter.cc

Comment 3 by mef@chromium.org, Apr 6 2017

- Does it need to be done for iOS as well?
- Could this be causing crash in b/34531911?

Cc: kapishnikov@chromium.org
Owner: kapishnikov@chromium.org
Status: Assigned (was: Started)
+ Andrei: 
I started https://codereview.chromium.org/2801603008/, but there was some subtle issue on iOS so I didn't get to finish it. 
Could you double check it is working as expected on iOS? Thanks!
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3c5a020f874bbd3a311ab325b541c4db2e100ac

commit a3c5a020f874bbd3a311ab325b541c4db2e100ac
Author: kapishnikov <kapishnikov@chromium.org>
Date: Wed May 17 20:51:26 2017

Make sure StatisticsRecorder::Initialize() is called at initialization

Also, add DCHECK that CronetEnvironment::Initialize() is called on the
main thread.

The current implementation guarantees that when [Cronet start] method
returns, the CronetEnvironment singleton is initialized, and, as the
result, base::StatisticsRecorder::Initialize() has been invoked.

BUG= 707836 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2885543002
Cr-Commit-Position: refs/heads/master@{#472555}

[modify] https://crrev.com/a3c5a020f874bbd3a311ab325b541c4db2e100ac/components/cronet/ios/cronet_environment.mm

Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b52fd86649e63521a591a46336a87a9fef9f3437

commit b52fd86649e63521a591a46336a87a9fef9f3437
Author: Misha Efimov <mef@chromium.org>
Date: Thu Jul 13 14:20:46 2017

[Cronet] Initialize StatisticsRecorder before TaskScheduler.

Task scheduler initialization gets histogram factories,
which require statistics recorder to be initialized.

Bug:  707836 
Change-Id: I01c1e5252720ba11b942f4120423fbe406713309
Reviewed-on: https://chromium-review.googlesource.com/568845
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486370}
[modify] https://crrev.com/b52fd86649e63521a591a46336a87a9fef9f3437/components/cronet/android/cronet_library_loader.cc
[modify] https://crrev.com/b52fd86649e63521a591a46336a87a9fef9f3437/components/cronet/ios/cronet_environment.mm

Sign in to add a comment