The following crash has a truncated stack: https://crash-stats.mozilla.com/report/index/71d49614-1793-4908-a2db-45d562110802 This happens because when analysing the _cairo_surface_composite frame we guess the wrong ebp and then when we get to _composite_trap_region we trust the wrong ebp don't find an eip and assume that we've reached the end of the stack. There are probably a number of ways to fix this. I've attached a patch that tries to avoid choosing the wrong ebp.

I seem to get the same stack with or without your patch on this dump. The only difference is the choice of ebp in the last frame: 5 xul.dll!_composite_trap_region [cairo-surface-fallback.c:e0521aa0fec0 : 522 + 0x22] - eip = 0x588a8e26 esp = 0x005ca2f8 ebp = 0x005ca338 + eip = 0x588a8e26 esp = 0x005ca2f8 ebp = 0x000001f4 Found by: call frame info with scanning

I guess this patch is needed too.

Comment on attachment 550600 [details] [diff] [review] Avoid guessing the wrong EBP. Review of attachment 550600 [details] [diff] [review]: ----------------------------------------------------------------- ::: src/processor/stackwalker_x86.cc @@ +413,5 @@ > + // > + // Note: it might be possible to actually disassemble the prologue if we > + // have the code to try to do a better job of finding ebp. > + // XXX: we shouldn't mark EBP as valid below > + } One thing about this code is that, although we accumulate a vector of candidates, we only ever use the first candidate, and the length of the vector. Could that be simplified? Also, this strategy seems delicate; it seems like it's tailored not only to that particular kind of prologue, but also to the specific caller (that happened to have stack addresses in those registers). It might make more sense to declare our findings ambiguous if there is more than one candidate, not only when *all* the words are candidates.

(In reply to comment #3) > Comment on attachment 550600 [details] [diff] [review] [diff] [details] [review] > Avoid guessing the wrong EBP. > > Review of attachment 550600 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > ::: src/processor/stackwalker_x86.cc > @@ +413,5 @@ > > + // > > + // Note: it might be possible to actually disassemble the prologue if we > > + // have the code to try to do a better job of finding ebp. > > + // XXX: we shouldn't mark EBP as valid below > > + } > > One thing about this code is that, although we accumulate a vector of > candidates, we only ever use the first candidate, and the length of the > vector. Could that be simplified? Certainly, I built around the idea of wanting to select an arbitrary candidate. But if we have no reason to do so, we should just keep the first one and count the rest. > Also, this strategy seems delicate; it seems like it's tailored not only to > that particular kind of prologue, but also to the specific caller (that > happened to have stack addresses in those registers). It is exactly that. I wrote it assuming that there were cases where we have multiple candidates and the first one is more likely to be correct than the rest. > It might make more sense to declare our findings ambiguous if there is more > than one candidate, not only when *all* the words are candidates. I think that would be great if we don't think it will break anything. Do you think we should try for that?

If you'd like, we could arrange to get a sample of minidumps from crash-stats and run a minidump_stackwalk with your patch against them and compare the output to stock mdsw.

Comment on attachment 550658 [details] [diff] [review] Scan further: increase the stack scanning limit to 50 words Review of attachment 550658 [details] [diff] [review]: ----------------------------------------------------------------- I'm not really wild about this. Stack scanning already gives us pretty crappy stacks, I feel like we're just giving it more opportunity to do so.

Comment on attachment 550600 [details] [diff] [review] Avoid guessing the wrong EBP. Review of attachment 550600 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to defer to jimb here, I'm not much of an expert on calling conventions.

Comment on attachment 550600 [details] [diff] [review] Avoid guessing the wrong EBP. Review of attachment 550600 [details] [diff] [review]: ----------------------------------------------------------------- I think, overall, this is not a patch we want to take. I don't feel like I understand 1) which situations it improves, and 2) the best other approach.