Closed Bug 730302 Opened 9 years ago Closed 9 years ago

Confusing and probably buggy code in TableTicker::Tick due to double declared i variable

Categories

(Core :: Gecko Profiler, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: mstange, Assigned: BenWa)

References

()

Details

Attachments

(1 file, 1 obsolete file)

TableTicker::Tick starts like this:

  int i = 0;
  const char *marker = mStack->getMarker(i++);
  for (int i = 0; marker != NULL; i++) {
    mProfile.addTag(ProfileEntry('m', marker));
    marker = mStack->getMarker(i++);
  }

Note that there are two "int i" declarations, and the i++ at the end of the loop probably increments the wrong i.

I'm confused about how this can work, if it works at all.
Attached patch patch (obsolete) — Splinter Review
You're right, good catch!
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #600395 - Flags: review?(mstange)
Comment on attachment 600395 [details] [diff] [review]
patch

Okay, I don't really understand what this code does. Why do we need to increment i twice in each iteration? Doesn't that mean we skip every second marker?

Assuming we want to add all markers to the profile, I'd write the code like this:

for (int i = 0; mStack->getMarker(i) != NULL; i++) {
  mProfile.addTag(ProfileEntry('m', mStack->getMarker(i)));
}

unless we really want to avoid calling getMarker twice.
Or can an entry in the stack's mMarkers array become NULL in the meantime due to access from a different thread?
Right, I guess I really wanted to use a while loop. We haven't used markers much so that why we haven't ran into this before. I'm glad that we're catching it now.
Attached patch patch v2Splinter Review
Attachment #600395 - Attachment is obsolete: true
Attachment #600399 - Flags: review?(mstange)
Attachment #600395 - Flags: review?(mstange)
Attachment #600399 - Flags: review?(mstange) → review+
Let us know if you're interested in hacking on the profiler. We need help either with c++, extension work, or the html front end.
I am and I'm looking at all three at the moment. :)
Not sure how much time I can spend on this, though... for now I'm just figuring out where we stand and how the existing things work.
Feel free to let me know if you have any questions or ping me on IRC for a quick summary of the overall design.

bz had some interesting suggestions for filtering samples on the front-end, and the switch to heavy stack doesn't work properly.
Will do, thanks!
https://hg.mozilla.org/mozilla-central/rev/ae55c1bf2a26
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.