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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

You're right, good catch!

Comment 2

[Markus Stange [: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.

Comment 3

[Markus Stange [:mstange]]

Or can an entry in the stack's mMarkers array become NULL in the meantime due to access from a different thread?

Comment 4

[Benoit Girard (:BenWa)]

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.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 6

[Benoit Girard (:BenWa)]

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.

Comment 7

[Markus Stange [:mstange]]

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.

Comment 8

[Benoit Girard (:BenWa)]

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.

Comment 9

[Markus Stange [:mstange]]

Will do, thanks!

Comment 10

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae55c1bf2a26

Comment 11

[Marco Bonardo [:mak]]

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