1467734 - Add ability to remove frames from CombinedStacks

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Carl Corcoran [:ccorcoran]]

In order to synchronize other structures with CombinedStacks, it needs the ability to remove items.

Comment 1

[Carl Corcoran [:ccorcoran]]

Attachment: Bug 1467734: Add ability to remove frames from CombinedStacks;

Review commit: https://reviewboard.mozilla.org/r/250242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250242/

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review257486

::: toolkit/components/telemetry/CombinedStacks.cpp:103
(Diff revision 1)
> +  mStacks.erase(mStacks.begin() + aIndex);
> +  if (mNextIndex > mStacks.size()) {
> +    mNextIndex = mStacks.size();

One concern here:
`mStacks` is a circular buffer and `mNextIndex` is used in `AddStack()` to reference the next writing location.
When `aIndex <= mNextIndex`, we'll also need to adjust the next writing location by one due to elements shifting?

Comment 3

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250242/diff/1-2/

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review259556

::: toolkit/components/telemetry/CombinedStacks.cpp:107
(Diff revision 2)
> +CombinedStacks::RemoveStack(unsigned aIndex) {
> +  MOZ_ASSERT(aIndex < mStacks.size());
> +
> +  mStacks.erase(mStacks.begin() + aIndex);
> +
> +  mNextIndex %= mMaxStacksCount;

Hey, probably a silly question here: why is this line needed?

I wish we had unit test coverage for this, but I filed bug 1471144 for it.

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review259558

::: commit-message-ea21b:1
(Diff revision 2)
> +Bug 1467734: Add ability to remove frames from CombinedStacks; r?aklotz

nit: let's update the message and drop aklotz from this :)

Comment 6

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250242/diff/2-3/

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review259556

> Hey, probably a silly question here: why is this line needed?
> 
> I wish we had unit test coverage for this, but I filed bug 1471144 for it.

I think MozReview probably discarded your comment on this :( Why is this line needed?

Comment 8

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review259556

> I think MozReview probably discarded your comment on this :( Why is this line needed?

mNextIndex is always incremented; the % is applied at
https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/CombinedStacks.cpp#39
to mod within bounds.

This line replicates the indexing behavior of CombinedStacks::AddStack

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review259556

> mNextIndex is always incremented; the % is applied at
> https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/CombinedStacks.cpp#39
> to mod within bounds.
> 
> This line replicates the indexing behavior of CombinedStacks::AddStack

> `mNextIndex` is always incremented;

In `AddStack`, right? Let's add a comment above this line mentioning why the modulo operator is needed *here*, since we're not incrementing `mNextIndex` in this function before this line, but we're adjusting it a couple of lines below.

Comment 10

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250242/diff/3-4/

Comment 11

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review259556

> > `mNextIndex` is always incremented;
> 
> In `AddStack`, right? Let's add a comment above this line mentioning why the modulo operator is needed *here*, since we're not incrementing `mNextIndex` in this function before this line, but we're adjusting it a couple of lines below.

Hmm I did one step further and just did the modulo in AddStack(). Looking at this class, there's no good reason it should ever be out of bounds like that. Simpler to just always keep it in bounds.

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

https://reviewboard.mozilla.org/r/250242/#review259634

This makes things clearer, thank you!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec11cc05b667
Add ability to remove frames from CombinedStacks; r=Dexter

Comment 14

[Cristina Coroiu [:ccoroiu]]

Backed out changeset ec11cc05b667 (bug 1467734) for xpcshell failures at toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js

Backout: https://hg.mozilla.org/integration/autoland/rev/2a84bd5f6e696bd92eeafa2b2757d15e9f911922

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ec11cc05b66710ce094679cfab92ab8149fde069

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185145197&repo=autoland&lineNumber=1840

task 2018-06-27T12:40:20.361Z] 12:40:20     INFO -  TEST-START | toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js
[task 2018-06-27T12:40:20.821Z] 12:40:20  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js | xpcshell return code: -11
[task 2018-06-27T12:40:20.822Z] 12:40:20     INFO -  TEST-INFO took 461ms
[task 2018-06-27T12:40:20.823Z] 12:40:20     INFO -  >>>>>>>
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  PID 7604 | [7604, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2682
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  TEST-PASS | toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js | run_test - [run_test : 90] true == true
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  TEST-PASS | toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js | run_test - [run_test : 91] 0 == 0
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  TEST-PASS | toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js | run_test - [run_test : 92] true == true
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  TEST-PASS | toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js | run_test - [run_test : 93] 0 == 0
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  (xpcshell/head.js) | test pending (2)
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  PID 7604 | Hit MOZ_CRASH(Bad canary value 0xf7690000 in FromData) at /builds/worker/workspace/build/src/xpcom/string/nsSubstring.cpp:378
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  PID 7604 | ExceptionHandler::GenerateDump cloned child 7619
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  PID 7604 | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  PID 7604 | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  running event loop
[task 2018-06-27T12:40:20.827Z] 12:40:20     INFO -  <<<<<<<
[task 2018-06-27T12:40:20.828Z] 12:40:20     INFO -  mozcrash Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/xpc-other-mRCCGf/487d2abf-9d0c-c24f-e515-ce1183f1360c.dmp /builds/worker/workspace/build/symbols
[task 2018-06-27T12:40:28.825Z] 12:40:28     INFO -  mozcrash Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/487d2abf-9d0c-c24f-e515-ce1183f1360c.dmp
[task 2018-06-27T12:40:28.826Z] 12:40:28     INFO -  mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/487d2abf-9d0c-c24f-e515-ce1183f1360c.extra
[task 2018-06-27T12:40:28.827Z] 12:40:28  WARNING -  PROCESS-CRASH | toolkit/components/telemetry/tests/unit/test_TelemetryLateWrites.js | application crashed [@ MOZ_CrashPrintf]
[task 2018-06-27T12:40:28.831Z] 12:40:28     INFO -  Crash dump filename: /tmp/xpc-other-mRCCGf/487d2abf-9d0c-c24f-e515-ce1183f1360c.dmp
[task 2018-06-27T12:40:28.832Z] 12:40:28     INFO -  Operating system: Linux
[task 2018-06-27T12:40:28.833Z] 12:40:28     INFO -                    0.0.0 Linux 4.4.0-1014-aws #14taskcluster1-Ubuntu SMP Tue Apr 3 10:27:00 UTC 2018 x86_64
[task 2018-06-27T12:40:28.834Z] 12:40:28     INFO -  CPU: x86
[task 2018-06-27T12:40:28.835Z] 12:40:28     INFO -       GenuineIntel family 6 model 62 stepping 4
[task 2018-06-27T12:40:28.836Z] 12:40:28     INFO -       2 CPUs
[task 2018-06-27T12:40:28.837Z] 12:40:28     INFO -  GPU: UNKNOWN
[task 2018-06-27T12:40:28.838Z] 12:40:28     INFO -  Crash reason:  SIGSEGV
[task 2018-06-27T12:40:28.839Z] 12:40:28     INFO -  Crash address: 0x0
[task 2018-06-27T12:40:28.841Z] 12:40:28     INFO -  Process uptime: not available
[task 2018-06-27T12:40:28.841Z] 12:40:28     INFO -  Thread 11 (crashed)

Comment 15

[Carl Corcoran [:ccorcoran]]

Apologies for the iteration on this. Typo; I'm skipping index 0 because of the order of lines. Fixing locally and doing thorough manual test.

I agree a unit test of CombinedStacks (et al) would be useful!

Updated patch & try run coming soon...

Comment 16

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8984409 [details]
Bug 1467734: Add ability to remove frames from CombinedStacks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250242/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af004da1539b
Add ability to remove frames from CombinedStacks; r=Dexter

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/af004da1539b