967589 - Failure in JS_NewRuntime() causes assertions while cleaning up

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: oom-test-new-runtime

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.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: oom-test-new-runtime v2

And remembering to qref this time...

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8370195 [details] [diff] [review]
oom-test-new-runtime

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.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 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.

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00c0dfc4c76b

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/00c0dfc4c76b