1735273 - Threads that were sleeping when profiler started could sometimes be left empty

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Found by code inspection (while working on bug 1577658), in ProfileBuffer::StreamSamplesToJSON :

      } else if (e.Has() && e.Get().IsTimeBeforeSameSample()) {
        if (sample.mTime == 0.0) {
          // We don't have any full sample yet, we cannot duplicate a "previous"
          // one. This should only happen at most once per thread, for the very
          // first sample.
          break;
        }

Notice the break. This means that if a thread has one of these "same samples" at the start of the buffer, we will stop streaming this thread.
Instead it should have been a continue, to keep streaming past that point, where there is probably a full sample.

This situation should be fairly rare, because it requires a delicate situation where the sampler is about to write a same sample entry, while the current buffer chunk is not full yet, but then when actually writing that entry, it crosses over to the next chunk.
But it would still be good to fix ASAP to avoid some data loss.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1735273 - Continue streaming thread after an initial same-as-before sample - r?canaltinova

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1633572

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Same question as bug 1735244 :)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/239ebc40e61c
Continue streaming thread after an initial same-as-before sample - r=canaltinova

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)

Same question as bug 1735244 :)

Which was:

Is there a real-world impact here which would justify backport consideration or can this fix ride the trains to release?

This issue is a bit more important: It would affect users of the Profiler, and could make some profiled threads silently disappear.
I think the issue should be fairly rare, but hard to detect, so it's difficult to gauge whether it's happening, but if it's happening it could mislead our users or our own developers.
And the fix is tiny. so I will request backport when possible...

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/239ebc40e61c

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 9245402 [details]
Bug 1735273 - Continue streaming thread after an initial same-as-before sample - r?canaltinova

Beta/Release Uplift Approval Request

• User impact if declined: Potential silent loss of data in captured profiles (threads would just be missing).
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This code would run in unlikely situations, that's why there are no tests because it would be very difficult to synthesize such situations.

However, I believe it is not risky:

• Only users of the profiler would be affected.
• This is only changing a break into a continue in the buffer processing loop,
• It should have been a continue from the start, it's new code that was recently added,
• There are other continues in the same loop, and most are exercised in tests; and the (re)start of the loop is then advancing to a specific type of entry, so it doesn't matter where this continue happens.
• In the worst case, if something did go wrong there against my judgement, I think it would probably be better than just silently dropping data.

• String changes made/needed:

Comment 8

[Dianna Smith [:diannaS]]

Comment on attachment 9245402 [details]
Bug 1735273 - Continue streaming thread after an initial same-as-before sample - r?canaltinova

Approved for 94.0b6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a4e6251a10c3