Failure in JS_NewRuntime() causes assertions while cleaning up

RESOLVED FIXED in mozilla30



JavaScript Engine
5 years ago
5 years ago


(Reporter: jonco, Assigned: jonco)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 2 obsolete attachments)

If JS_NewRuntime() fails due to (for example due to OOM), it calls JS_DestroyRuntime() to clean up the partially-created runtime.  However this triggers assertions in several places where it assumes the runtime has been fully created.

This is showing up where a GGC crash in a Windows build is causing this to happen, which I'm guessing is masking a real problem.
Blocks: 964340
Created attachment 8370195 [details] [diff] [review]

Here's a patch to add an OOM test for JS_NewRuntime() and fix the problems it showed up.

These are mainly either incorrect assertions or trying to destroy things which aren't necessarily initialized.

I had to add an 'initialized' member to JSRuntime as the seemed the simplest way to check whether a final GC was required.
Attachment #8370195 - Flags: review?(wmccloskey)
Created attachment 8370197 [details] [diff] [review]
oom-test-new-runtime v2

And remembering to qref this time...
Attachment #8370195 - Attachment is obsolete: true
Attachment #8370195 - Flags: review?(wmccloskey)
Attachment #8370197 - Flags: review?(wmccloskey)
Comment on attachment 8370195 [details] [diff] [review]

Review of attachment 8370195 [details] [diff] [review]:

I think it might be easier if you set initialized_ after the atoms compartment is created and right before InitAtoms. That way, js_FinishGC can just return immediately if !initialized_. Then I don't think you will need to change the zone iterators at all, since I don't think we'll call them if !initialized_. And if initialized_ is true, we'll have an atoms compartment, so the special cases shouldn't be needed.

::: js/src/jsapi-tests/testOOM.cpp
@@ +27,5 @@
>  END_TEST(testOOM)
> +
> +const uint32_t maxAllocsPerTest = 100;
> +
> +#define START_OOM_TEST(name)                                                  \

Why do you need the macros? They're only used once. Do you plan to add more tests like this? If so, I wonder if we could do it with function pointers or something.

::: js/src/jsgcinlines.h
@@ +320,5 @@
>      ZonesIter zone;
>    public:
>      GCZonesIter(JSRuntime *rt) : zone(rt, WithAtoms) {
> +        if (!done() && !zone->isCollecting())

Why do we need this? GCZonesIter should only be used during GC, and I don't see how we would GC in a runtime that hasn't been initialized.
Created attachment 8372216 [details] [diff] [review]
oom-test-new-runtime v3

Ok, how about this.  We set a gcInitialized flag at the point that everything that the collector depends on is initialized.  js_FinishGC() still has to destroy anything that did get initialized, but we can use this to stop it iterating zones and triggering that assertion.

~JSRuntime() currently iterates compartments in a couple of places which in turn iterates Zones, so we need to stop any of that happening if !gcInitialized and then we don't need to change the iterators at all.

> +#define START_OOM_TEST(name)    

I did this as a macro so we could add more of these in future.  Using function pointers is also possible but means you have to use global variables for any state you want to share between tests and obscures the control flow.
Attachment #8370197 - Attachment is obsolete: true
Attachment #8370197 - Flags: review?(wmccloskey)
Attachment #8372216 - Flags: review?(wmccloskey)
Attachment #8372216 - Flags: review?(wmccloskey) → review+
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.