Large stack traces can hang browser for a long time

RESOLVED FIXED in mozilla9

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: billm, Assigned: luke)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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.
(Reporter)

Comment 2

6 years ago
Created attachment 551152 [details]
second trace

This one seems to happen when we try to free the massive exception stack trace recursively.
(Reporter)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
(Reporter)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
Ah, I forgot that iteration was building the exception object; I thought it was iterating to... I don't know what I was thinking.
(Assignee)

Comment 7

6 years ago
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...
Attachment #554952 - Flags: feedback?(mrbkap)
(Assignee)

Comment 8

6 years ago
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.
Attachment #554952 - Flags: feedback?(mrbkap)
(Assignee)

Updated

6 years ago
Attachment #554952 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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://stage.mozilla.org/pub/mozilla.org/firefox/try-builds/lwagner@mozilla.com-381a04c7117b/try-win32/firefox-9.0a1.en-US.win32.zip
(Assignee)

Comment 11

6 years ago
Still green.  Waiting for verification of the fix before requesting review.
(Assignee)

Comment 12

6 years ago
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.
Attachment #555862 - Flags: review?(mrbkap)
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!
Attachment #555862 - Flags: review?(mrbkap) → review+
Blocks: 682758
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/363c40e06667
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/363c40e06667
Assignee: nobody → luke
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Version: unspecified → Trunk

Comment 16

6 years ago
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
You need to log in before you can comment on or make changes to this bug.