Last Comment Bug 715634 - Cleanup XPCJSContextStack
: Cleanup XPCJSContextStack
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-05 12:53 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-01-11 04:58 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (29.50 KB, patch)
2012-01-05 12:53 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2 (29.39 KB, patch)
2012-01-06 04:12 PST, :Ms2ger (⌚ UTC+1/+2)
bobbyholley: review+
Details | Diff | Splinter Review
Patch v2 (diff -w) (24.97 KB, patch)
2012-01-06 15:11 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-01-05 12:53:55 PST
Created attachment 586188 [details] [diff] [review]
Patch v1

For some reason, XPCJSContextStack uses virtual methods with useless nsresult returns... This patch should make stuff cleaner.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-01-06 04:12:27 PST
Created attachment 586385 [details] [diff] [review]
Patch v2

And apparently people call Pop with a null retval
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-01-06 15:11:05 PST
Created attachment 586581 [details] [diff] [review]
Patch v2 (diff -w)

And in case a diff -w helps you, here's one.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-01-06 17:20:45 PST
Comment on attachment 586385 [details] [diff] [review]
Patch v2

>+    nsRefPtr<nsXPConnect> xpc = nsXPConnect::GetXPConnect();
>+    if (!xpc)
>+        return NULL;
>+
>+    XPCJSRuntime* xpcrt = xpc->GetRuntime();
>+    if (!xpcrt)
>+        return NULL;
>+
>+    JSRuntime *rt = xpcrt->GetJSRuntime();
>+    if (!xpcrt)
>+        return NULL;

This should check rt, not xpcrt.

> private:
>-    nsAutoTArray<XPCJSContextInfo, 16> mStack;
>+    AutoFallibleTArray<XPCJSContextInfo, 16> mStack;

What's this all about?

r+ with the above fix and a reasonable explanation for the FallibleT stuff.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-01-07 00:22:06 PST
The code was checking for allocation failures, even though nsTArray / nsAutoTArray changed to using infallible malloc a while ago. Because we don't gain much from using infallible malloc (we still have to handle failure from JS_SaveFrameChain), I thought we might as well use explicitly fallible malloc.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-01-07 00:58:49 PST
(In reply to Ms2ger from comment #4)
> The code was checking for allocation failures, even though nsTArray /
> nsAutoTArray changed to using infallible malloc a while ago. Because we
> don't gain much from using infallible malloc (we still have to handle
> failure from JS_SaveFrameChain), I thought we might as well use explicitly
> fallible malloc.

Hm. Given that this is never going to be allocating much more than 200 bytes, I'd prefer to keep this infallible. IMO fallible allocation should be reserved for large (or potentially large) allocations. OOMing on allocations of these size just means the browser is about to fall flat on its face anyway, so I'd rather keep the code simple.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-01-07 01:20:59 PST
WFM. I'll make it AutoInfallibleTArray for clarity and remove the checks, then.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-01-11 04:58:35 PST
https://hg.mozilla.org/mozilla-central/rev/df571c75b4c0

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