Closed Bug 1145333 Opened 5 years ago Closed 5 years ago

Skip nativeStackAddrs that conflict with pseudoStackAddrs and jsStackAddrs in mergeStacksIntoProfile

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: djvj, Assigned: djvj)

References

Details

Attachments

(1 file, 1 obsolete file)

Sometimes, in mergeStacksIntoProfile in TableTicker.cpp, we can get native stack addrs that are the same as a pseudstack addr, or a js stack addr.

Currently, we assert that this does not happen, but it can happen in rare cases.  We should handle this case instead of just asserting it, and give preference to pseudostack or jsstack interpretations over the nativestack interpretation.
Blocks: 1141712
QA Contact: kvijayan
Attachment #8580251 - Flags: review?(mstange)
Updated patch with comment.
Attachment #8580251 - Attachment is obsolete: true
Attachment #8580251 - Flags: review?(mstange)
Attachment #8580253 - Flags: review?(mstange)
Attachment #8580253 - Flags: review?(mstange) → review+
Even with that in place, I've seen occasional assertion failures further down,
here:

    // If we reach here, there must be a native stack entry and it must be the
    // greatest entry.
    MOZ_ASSERT(nativeStackAddr);  <----------------- HERE
    MOZ_ASSERT(nativeIndex >= 0);
    aProfile.addTag(ProfileEntry('l', (void*)aNativeStack.pc_array[nativeIndex]));
    nativeIndex--;

I suspect it is possible to get to this point with nativeStackAddr,
jsStackAddr, pseudoStackAddr all being zero.  Does that sound
plausible?  If we have all 3 being zero, then this doesn't apply

    // Check to see if pseudoStack frame is top-most.
    if (pseudoStackAddr > jsStackAddr && pseudoStackAddr > nativeStackAddr) {

and neither does this

    // Check to see if JS jit stack frame is top-most
    if (jsStackAddr > nativeStackAddr) {

so we wind up failing here:

    MOZ_ASSERT(nativeStackAddr);

It's very difficult to reproduce, though -- I've only seen it about
three or four times.  So I can't yet confirm the all-3-values-zero
hypothesis.
(In reply to Julian Seward [:jseward] from comment #4)
> I suspect it is possible to get to this point with nativeStackAddr,
> jsStackAddr, pseudoStackAddr all being zero.

Verified:

    // If we reach here, there must be a native stack entry and it must be the
    // greatest entry.
if (!nativeStackAddr)
fprintf(stderr, "XXXXXXXXXXXXXXXXXXXXX %p %p %p\n", nativeStackAddr, jsStackAddr, pseudoStackAddr);
    MOZ_ASSERT(nativeStackAddr);

produces

XXXXXXXXXXXXXXXXXXXXX (nil) (nil) (nil)
Assertion failure: nativeStackAddr, at /home/sewardj/MOZ/MC-PROF/tools/profiler/TableTicker.cpp:626
Any opinions about this as a fix?

     // If we reach here, there must be a native stack entry and it must be the
     // greatest entry.
-    MOZ_ASSERT(nativeStackAddr);
-    MOZ_ASSERT(nativeIndex >= 0);
-    aProfile.addTag(ProfileEntry('l', (void*)aNativeStack.pc_array[nativeIndex]));
-    nativeIndex--;
+    if (nativeStackAddr) {
+      MOZ_ASSERT(nativeIndex >= 0);
+      aProfile.addTag(ProfileEntry('l', (void*)aNativeStack.pc_array[nativeIndex]));
+    }
+    if (nativeIndex >= 0) {
+      nativeIndex--;
+    }
https://hg.mozilla.org/mozilla-central/rev/3abdf236d4f2
Assignee: nobody → kvijayan
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.