Cleanup XPCJSContextStack

RESOLVED FIXED in mozilla12

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Attachment #586188 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 1

6 years ago
Created attachment 586385 [details] [diff] [review]
Patch v2

And apparently people call Pop with a null retval
Attachment #586188 - Attachment is obsolete: true
Attachment #586188 - Flags: review?(bobbyholley+bmo)
Attachment #586385 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 2

6 years ago
Created attachment 586581 [details] [diff] [review]
Patch v2 (diff -w)

And in case a diff -w helps you, here's one.
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.
Attachment #586385 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 4

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

Comment 6

6 years ago
WFM. I'll make it AutoInfallibleTArray for clarity and remove the checks, then.
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/df571c75b4c0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.