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

Issue 637129 link

Starred by 1 user
Status: Fixed
Owner:
oysteine@chromium.org
Closed: Nov 2016
Cc:
ssid@chromium.org
oysteine@chromium.org
primiano@chromium.org
█ picksi@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 634089



Sign in to add a comment

MemoryInfra: Background traces miss browser process

Project Member Reported by ssid@chromium.org, Aug 11 2016 
Background context:go/memory-infra: memory profiling in chrome://tracing

the browser process dump will go missing from traces in cases where the tracing is stopped before the dumps are completed. This could happen because on consecutive memory pressure signals the tracing will start and stop immediately.
On REACTIVE tracing mode the tracing should not stop with the UMA metric trigger.

Comment 1 by petrcermak@chromium.org, Aug 12 2016

Cc: primiano@chromium.org oysteine@chromium.org
I think this should be fixed by modifying Chrome to postpone stopping tracing until the ongoing memory dump finishes (rejecting any other events from being added to the trace).

Comment 2 by oysteine@chromium.org, Aug 12 2016 

If the UMA trigger hits while tracing is active, as you mentioned the current behavior is to stop tracing. This made sense for navigation triggers for Deep Reports (i.e. because a new navigation is starting and then we want a separate trace), but I agree it doesn't make sense in this situation.

Would making that configurable reduce this problem enough? Adding mechanisms to delay trace stops until XYZ has finished sounds potentially complex.

Comment 3 by ssid@chromium.org, Aug 12 2016

Cc: petrcermak@chromium.org
There are 2 different issues here:
1. The background tracing manager can potentially stop tracing very quickly after it starts and traces could miss dumps. This should be easily fixed by avoiding stopping for REACTIVE triggers.

I do not think Petr is suggesting that the background tracing manager should depend on memory-infra finishing.

2. The telemetry always expects the global dumps to have all processes alive (especially browser) while memory-infra is designed in such a way that this is not the case. Ideally we could find a way to throw away the dumps that do not contain browser. But, this is difficult to coordinate between the processes since the dumps are added as trace events by each process. I was thinking it is easier to make telemetry robust to such cases.

Comment 4 by oysteine@chromium.org, Aug 13 2016 

Yeah that sounds pretty reasonable to me!

I'll put together a quick CL to address #1.

Comment 5 by petrcermak@chromium.org, Aug 15 2016 

My proposal was to let the current memory-infra dump finish before stopping tracing. No other trace events should be inserted and there should probably be a timeout (in case of a memory-infra bug).

Comment 6 by ssid@chromium.org, Aug 15 2016 

It is not a good idea to couple memory-infra with tracing. They are 2 different components. Considering all possible cases where start and stop tracing happens and if we delay stop tracing till memory dump finishes, there could be cases like the stop button after record in the about:tracing UI will go unresponsive if some dump provider did take few seconds to dump.
For the case of background tracing, after the fix that oysteine@ is talking about in #4, the only code path that calls StopTracing will be a timeout. Note that the issue might still occur if timeout is hit and the browser still did not finish dump.

This is an universal issue that a process does not finish dumping when tracing stopped. We should either assume that processes can go missing in the last dump or memory-infra should not add the incomplete global dump to traces.
Also there is no mechanism to delay stop tracing from renderer, if renderer did not finish the dump. It might also be hard to differentiate between a dead renderer that did not finish dump and a renderer that is yet to complete the last dump.

Comment 7 by petrcermak@chromium.org, Aug 16 2016 

I see your point. However, I am against special-casing the logic for the last dump. If it's a common problem (that can't be fixed on the Chrome side), then I suggest memory-infra dumps a memory-dump-finished event after all processes have ACKed (in Chrome) and then we ignore all memory dumps that don't have an associated memory-dump-finished event (in Catapult).
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 23 2016 

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

commit ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897
Author: megjablon <megjablon@chromium.org>
Date: Tue Aug 23 01:57:28 2016

Revert of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2247033005/ )

Reason for revert:
Test is flaky on builder "Android Tests"

Builder: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests

Failure:
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/30899

Output:
BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored (run #1):
[ RUN      ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored
[WARNING:dns_config_service_posix.cc(316)] Failed to read DnsConfig.
[ERROR:devtools_http_handler.cc(228)] Cannot start http server for devtools. Stop devtools.
../../content/browser/tracing/background_tracing_manager_browsertest.cc:1210: Failure
Value of: BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting()
  Actual: false
Expected: true
../../content/browser/tracing/background_tracing_manager_browsertest.cc:95: Failure
Value of: value
  Actual: true
Expected: expected
Which is: false
[  FAILED  ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored, where TypeParam =  and GetParam() =  (188 ms)
[----------] 1 test from BackgroundTracingManagerBrowserTest (188 ms total)

Original issue's description:
> Background tracing: Add config option to control whether or not to stop reactive tracing when a repeated trigger occurs
>
> R=simonhatch@chromium.org
> BUG= 637129 
>
> Committed: https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
> Cr-Commit-Position: refs/heads/master@{#412577}

TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,oysteine@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 637129 

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

[modify] https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897/chrome/browser/tracing/navigation_tracing.cc
[modify] https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897/content/browser/tracing/background_tracing_config_unittest.cc
[modify] https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897/content/browser/tracing/background_tracing_manager_browsertest.cc
[modify] https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897/content/browser/tracing/background_tracing_manager_impl.cc
[modify] https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897/content/browser/tracing/background_tracing_rule.cc
[modify] https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897/content/browser/tracing/background_tracing_rule.h

Comment 11 by ssid@chromium.org, Aug 25 2016

Blocking: 634089

Comment 12 by ssid@chromium.org, Aug 25 2016

Cc: ssid@chromium.org
Owner: oysteine@chromium.org
Assigning to Oystein to fix the background tracing REACTIVE mode. Please assign it back to me when done. Thanks.
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 27 2016 

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

commit 2f4c6f2c22a02a172845c8beb9c4effb2389af13
Author: oysteine <oysteine@chromium.org>
Date: Sat Aug 27 00:04:40 2016

Reland of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2265423002/ )

Reason for revert:
Re-landing with fix

Original issue's description:
> Revert of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2247033005/ )
>
> Reason for revert:
> Test is flaky on builder "Android Tests"
>
> Builder: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests
>
> Failure:
> https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/30899
>
> Output:
> BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored (run #1):
> [ RUN      ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored
> [WARNING:dns_config_service_posix.cc(316)] Failed to read DnsConfig.
> [ERROR:devtools_http_handler.cc(228)] Cannot start http server for devtools. Stop devtools.
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:1210: Failure
> Value of: BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting()
>   Actual: false
> Expected: true
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:95: Failure
> Value of: value
>   Actual: true
> Expected: expected
> Which is: false
> [  FAILED  ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored, where TypeParam =  and GetParam() =  (188 ms)
> [----------] 1 test from BackgroundTracingManagerBrowserTest (188 ms total)
>
> Original issue's description:
> > Background tracing: Add config option to control whether or not to stop reactive tracing when a repeated trigger occurs
> >
> > R=simonhatch@chromium.org
> > BUG= 637129 
> >
> > Committed: https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
> > Cr-Commit-Position: refs/heads/master@{#412577}
>
> TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,oysteine@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 637129 
>
> Committed: https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897
> Cr-Commit-Position: refs/heads/master@{#413627}

TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,megjablon@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 637129 

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

[modify] https://crrev.com/2f4c6f2c22a02a172845c8beb9c4effb2389af13/chrome/browser/tracing/navigation_tracing.cc
[modify] https://crrev.com/2f4c6f2c22a02a172845c8beb9c4effb2389af13/content/browser/tracing/background_tracing_config_unittest.cc
[modify] https://crrev.com/2f4c6f2c22a02a172845c8beb9c4effb2389af13/content/browser/tracing/background_tracing_manager_browsertest.cc
[modify] https://crrev.com/2f4c6f2c22a02a172845c8beb9c4effb2389af13/content/browser/tracing/background_tracing_manager_impl.cc
[modify] https://crrev.com/2f4c6f2c22a02a172845c8beb9c4effb2389af13/content/browser/tracing/background_tracing_rule.cc
[modify] https://crrev.com/2f4c6f2c22a02a172845c8beb9c4effb2389af13/content/browser/tracing/background_tracing_rule.h

Comment 14 by oysteine@chromium.org, Aug 30 2016

Labels: Merge-Request-54
Requesting merge to M54 as this is needed for data from Android Beta and reland seems to be sticking.

Comment 15 by dimu@chromium.org, Aug 30 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 30 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b997559f8b767b1f74cd1f8c8eed408a99e52843

commit b997559f8b767b1f74cd1f8c8eed408a99e52843
Author: Oystein Eftevaag <oysteine@chromium.org>
Date: Tue Aug 30 21:42:24 2016

Reland of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2265423002/ )

Reason for revert:
Re-landing with fix

Original issue's description:
> Revert of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2247033005/ )
>
> Reason for revert:
> Test is flaky on builder "Android Tests"
>
> Builder: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests
>
> Failure:
> https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/30899
>
> Output:
> BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored (run #1):
> [ RUN      ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored
> [WARNING:dns_config_service_posix.cc(316)] Failed to read DnsConfig.
> [ERROR:devtools_http_handler.cc(228)] Cannot start http server for devtools. Stop devtools.
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:1210: Failure
> Value of: BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting()
>   Actual: false
> Expected: true
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:95: Failure
> Value of: value
>   Actual: true
> Expected: expected
> Which is: false
> [  FAILED  ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored, where TypeParam =  and GetParam() =  (188 ms)
> [----------] 1 test from BackgroundTracingManagerBrowserTest (188 ms total)
>
> Original issue's description:
> > Background tracing: Add config option to control whether or not to stop reactive tracing when a repeated trigger occurs
> >
> > R=simonhatch@chromium.org
> > BUG= 637129 
> >
> > Committed: https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
> > Cr-Commit-Position: refs/heads/master@{#412577}
>
> TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,oysteine@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 637129 
>
> Committed: https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897
> Cr-Commit-Position: refs/heads/master@{#413627}

TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,megjablon@chromium.org
BUG= 637129 

Review-Url: https://codereview.chromium.org/2285853002
Cr-Commit-Position: refs/heads/master@{#414867}
(cherry picked from commit 2f4c6f2c22a02a172845c8beb9c4effb2389af13)

Review URL: https://codereview.chromium.org/2294093002 .

Cr-Commit-Position: refs/branch-heads/2840@{#54}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/chrome/browser/tracing/navigation_tracing.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_config_unittest.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_manager_browsertest.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_manager_impl.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_rule.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_rule.h

Comment 17 by petrcermak@chromium.org, Oct 5 2016

Cc: -petrcermak@chromium.org

Comment 18 by petrcermak@chromium.org, Oct 5 2016

Cc: -petrcermak@chromium.org
Project Member

Comment 19 by bugdroid1@chromium.org, Oct 27 2016 

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

commit b997559f8b767b1f74cd1f8c8eed408a99e52843
Author: Oystein Eftevaag <oysteine@chromium.org>
Date: Tue Aug 30 21:42:24 2016

Reland of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2265423002/ )

Reason for revert:
Re-landing with fix

Original issue's description:
> Revert of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2247033005/ )
>
> Reason for revert:
> Test is flaky on builder "Android Tests"
>
> Builder: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests
>
> Failure:
> https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/30899
>
> Output:
> BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored (run #1):
> [ RUN      ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored
> [WARNING:dns_config_service_posix.cc(316)] Failed to read DnsConfig.
> [ERROR:devtools_http_handler.cc(228)] Cannot start http server for devtools. Stop devtools.
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:1210: Failure
> Value of: BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting()
>   Actual: false
> Expected: true
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:95: Failure
> Value of: value
>   Actual: true
> Expected: expected
> Which is: false
> [  FAILED  ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored, where TypeParam =  and GetParam() =  (188 ms)
> [----------] 1 test from BackgroundTracingManagerBrowserTest (188 ms total)
>
> Original issue's description:
> > Background tracing: Add config option to control whether or not to stop reactive tracing when a repeated trigger occurs
> >
> > R=simonhatch@chromium.org
> > BUG= 637129 
> >
> > Committed: https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
> > Cr-Commit-Position: refs/heads/master@{#412577}
>
> TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,oysteine@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 637129 
>
> Committed: https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897
> Cr-Commit-Position: refs/heads/master@{#413627}

TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,megjablon@chromium.org
BUG= 637129 

Review-Url: https://codereview.chromium.org/2285853002
Cr-Commit-Position: refs/heads/master@{#414867}
(cherry picked from commit 2f4c6f2c22a02a172845c8beb9c4effb2389af13)

Review URL: https://codereview.chromium.org/2294093002 .

Cr-Commit-Position: refs/branch-heads/2840@{#54}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/chrome/browser/tracing/navigation_tracing.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_config_unittest.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_manager_browsertest.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_manager_impl.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_rule.cc
[modify] https://crrev.com/b997559f8b767b1f74cd1f8c8eed408a99e52843/content/browser/tracing/background_tracing_rule.h

Comment 20 by primiano@chromium.org, Nov 25 2016 

triaging pass: is this fixed?

Comment 21 by oysteine@chromium.org, Nov 28 2016

Status: Fixed (was: Untriaged)
Yeap.

Comment 22 by primiano@chromium.org, Jan 4 2017

Components: Internals>Instrumentation>Memory
►

Sign in to add a comment