Closed Bug 1467734 Opened 4 years ago Closed 4 years ago

Add ability to remove frames from CombinedStacks

Categories

(Toolkit :: Telemetry, enhancement, P1)

Unspecified
Windows
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ccorcoran, Assigned: ccorcoran)

References

Details

Attachments

(1 file)

In order to synchronize other structures with CombinedStacks, it needs the ability to remove items.
Component: General → Telemetry
Product: Firefox → Toolkit
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?
Attachment #8984409 - Flags: review?(gfritzsche)
Attachment #8984409 - Flags: review?(aklotz) → review?(alessio.placitelli)
See Also: → 1471144
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 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 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 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 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 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 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!
Attachment #8984409 - Flags: review?(alessio.placitelli) → review+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec11cc05b667
Add ability to remove frames from CombinedStacks; r=Dexter
Keywords: checkin-needed
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)
Flags: needinfo?(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...
Flags: needinfo?(ccorcoran)
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af004da1539b
Add ability to remove frames from CombinedStacks; r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/af004da1539b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.