Last Comment Bug 538143 - [@ XPCJSStackFrame::CreateStack] should not use recursion
: [@ XPCJSStackFrame::CreateStack] should not use recursion
Status: RESOLVED FIXED
fixed-in-tracemonkey
: crash
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: timeless
:
Mentors:
: 538050 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-06 05:30 PST by timeless
Modified: 2011-06-13 10:01 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
switch away from recursion (4.05 KB, patch)
2010-01-06 05:32 PST, timeless
mrbkap: review-
Details | Diff | Splinter Review
use refptr (4.22 KB, patch)
2010-03-06 12:47 PST, timeless
mrbkap: review-
Details | Diff | Splinter Review
addresses comments (7.33 KB, patch)
2011-03-31 00:41 PDT, timeless
mrbkap: review+
Details | Diff | Splinter Review

Description timeless 2010-01-06 05:30:17 PST
this is bug 303821 again.

I'm tired of reading this code and think it's time to switch to iteration.
Comment 1 timeless 2010-01-06 05:32:57 PST
Created attachment 420309 [details] [diff] [review]
switch away from recursion
Comment 2 timeless 2010-01-06 05:34:16 PST
*** Bug 538050 has been marked as a duplicate of this bug. ***
Comment 3 Blake Kaplan (:mrbkap) 2010-03-02 15:24:14 PST
Comment on attachment 420309 [details] [diff] [review]
switch away from recursion

>diff --git a/js/src/xpconnect/src/xpcstack.cpp b/js/src/xpconnect/src/xpcstack.cpp

So, we haven't yet gone to infallible new, so IMO you should deal with it.

> XPCJSStackFrame::CreateStack(JSContext* cx, JSStackFrame* fp,
>                              XPCJSStackFrame** stack)
> {
>-    XPCJSStackFrame* self = new XPCJSStackFrame();
>-    JSBool failed = JS_FALSE;
>-    if(self)
>+    XPCJSStackFrame* first = new XPCJSStackFrame();
>+    XPCJSStackFrame* self = first;

Any reason to not make self and first nsRefPtrs?

>+    while (fp && self)

Nit: no space before the `('.

>     {
>         NS_ADDREF(self);
> 
>-        if(fp->down)
>+        if (JS_IsNativeFrame(cx, fp))

Ditto. And there's one more spot below.

r- until the checks for OOM have been dealt with (feel free to correct me if I'm off base by asking for this).
Comment 4 timeless 2010-03-06 12:47:26 PST
Created attachment 430869 [details] [diff] [review]
use refptr
Comment 5 Blake Kaplan (:mrbkap) 2010-03-15 20:55:59 PDT
Comment on attachment 430869 [details] [diff] [review]
use refptr

>+    nsRefPtr<XPCJSStackFrame> first = new XPCJSStackFrame();
>+    if(!first)
>+        return NS_ERROR_OUT_OF_MEMORY;

Heh, of course now that you've re-requested review, infallible new has landed and you can get rid of this. Sorry.

>+        if((fp = fp->down))
>+            XPCJSStackFrame* frame = new XPCJSStackFrame();
>+            self->mCaller = frame;
>+            self = frame;

This seems wrong. self->mCaller isn't a smart pointer (though perhaps it should be!) so this looks like it'll end up freeing self->mCaller too early.

This actually suggests that we just make |self| a raw pointer and let the refptr to |first| and its mCaller chain represent the strong pointers to the whole chain.

>+    NS_ADDREF(*stack = first);

This could just be |*stack = first.forget();|
Comment 6 timeless 2011-03-31 00:41:52 PDT
Created attachment 523244 [details] [diff] [review]
addresses comments

sorry for the delay. this has been maintained in my queue, and i had addressed most of those items except for the oom check and the forget bit. i've now added them as well.

please do whatever is necessary to get this landed. do not wait for me to address further review comments. this bug is really closer to a decade old than the year of lag on my part makes it look (i've actually written this patch before at other companies too possibly both around 2002 and 2004). it's actually somewhat important as you end up crashing in cases with deep stacks when something wants to report a problem with them.
Comment 7 Blake Kaplan (:mrbkap) 2011-05-16 04:52:21 PDT
Comment on attachment 523244 [details] [diff] [review]
addresses comments

Review of attachment 523244 [details] [diff] [review]:
-----------------------------------------------------------------

I fixed a merge conflict locally. I'll check this in as soon as tracemonkey reopens.
Comment 8 Blake Kaplan (:mrbkap) 2011-05-17 06:56:02 PDT
http://hg.mozilla.org/tracemonkey/rev/ece0feb51587
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:11:28 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/ece0feb51587

Note You need to log in before you can comment on or make changes to this bug.