Dvander gave me some stack traces from someone who gets Firefox hangs pretty often. Both of them seem related to exceptions and big stack traces. I'll attach them below.
Created attachment 551148 [details] first stack trace This one seems to be caused by inefficient stack frame iteration. The relevant code seems to be XPCJSStackFrame::CreateStack.
Created attachment 551152 [details] second trace This one seems to happen when we try to free the massive exception stack trace recursively.
The obvious solution here is to add some sort of cutoff to the number of frames we capture. Luke said that this problem may have gotten worse recently because the JS engine increased the max number of frames. Blake, how much freedom do we have here? You mentioned that there are some web interfaces that we have to support. I'm sure that we could make the algorithms and data structures more efficient. But if we have to build up a giant stack trace object every time someone feels like writing a recursive infinite loop, it's not going to do much good.
To be clear: for the case of stack iteration, we don't need a cutoff, we just need to not use a quadratic iteration algorithm (i.e., StackFrame::pcQuadratic) when n = 50,000.
(In reply to Luke Wagner [:luke] from comment #4) > To be clear: for the case of stack iteration, we don't need a cutoff, we > just need to not use a quadratic iteration algorithm (i.e., > StackFrame::pcQuadratic) when n = 50,000. That would make the problem a lot better. But it seems like we still don't want to be creating giant stack trace objects that the user is unlikely to want. And if we have a cutoff, it would be during creation.
Ah, I forgot that iteration was building the exception object; I thought it was iterating to... I don't know what I was thinking.
Created attachment 554952 [details] [diff] [review] quick fix Stacks can get quite large now (50K frames). If the only issue was pcQuadratic from JS_GetFramePC, we could just switch to FrameRegsIter. But it seems like the time to create all these XPCJSStackFrames is also a big deal. This patch just caps the number of stack frames we create. This seems fine as long as code isn't depending on the contents of the stack for some hard security judgment or defined by a spec. Blake, do you know anything about this? Pushing to try...
Very green. Now all we need to know is is it totally wrong ;-)
Comment on attachment 554952 [details] [diff] [review] quick fix I'm fine with this. I don't see where you increment numFrames, though (also: no space after if in XPConnect). No security code that I know of uses this code. Even with this fix, though, I think we should file a followup bug on trying to clean this up to be less of a memory hog. I don't see a reason why we need an XPCOM object per stack frame. That seems wasteful, especially in C++. So, we should file a followup bug on cleaning that up, either by changing the API to be more of an iterator API or making the current implementation defer work until it's needed or other similar optimizations.
This time with the increment ;-). Still green on try. dvander, can you or the other fellow verify the fix: wget --progress=dot:mega -N http://firstname.lastname@example.org/try-win32/firefox-9.0a1.en-US.win32.zip
Still green. Waiting for verification of the fix before requesting review.
Created attachment 555862 [details] [diff] [review] limit stack depth when building XPC stack David said that the reporter used the build for a day and the hangs went away.
Comment on attachment 555862 [details] [diff] [review] limit stack depth when building XPC stack Review of attachment 555862 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/xpconnect/src/xpcstack.cpp @@ +189,5 @@ > self->mLanguage = nsIProgrammingLanguage::CPLUSPLUS; > } > } > > + if (++numFrames > MAX_FRAMES) if( please: no space after if!
Created attachment 567715 [details] testcase http://www.in-town.nl/web/ appears to exercise this on Beta/8. Reduction crashes Beta/8 debug but not opt on Mac OS X 10.5