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
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-05 12:53 PST by :Ms2ger
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
no flags Details | Diff | Review
Patch v2 (29.39 KB, patch)
2012-01-06 04:12 PST, :Ms2ger
bobbyholley: review+
Details | Diff | Review
Patch v2 (diff -w) (24.97 KB, patch)
2012-01-06 15:11 PST, :Ms2ger
no flags Details | Diff | Review

Description :Ms2ger 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 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 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 (PTO through June 13) 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 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 (PTO through June 13) 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 2012-01-07 01:20:59 PST
WFM. I'll make it AutoInfallibleTArray for clarity and remove the checks, then.

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