715634 - Cleanup XPCJSContextStack

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

For some reason, XPCJSContextStack uses virtual methods with useless nsresult returns... This patch should make stuff cleaner.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

And apparently people call Pop with a null retval

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2 (diff -w)

And in case a diff -w helps you, here's one.

Comment 3

[Bobby Holley (:bholley)]

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 (he/him; ⌚ UTC+1/+2)]

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)]

(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 (he/him; ⌚ UTC+1/+2)]

WFM. I'll make it AutoInfallibleTArray for clarity and remove the checks, then.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/df571c75b4c0