523370 - bogus OOM with empty double free lists

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

With changes from the bug 504797 jstracer.cpp NativeToValueBase contains:

            if (JS_THREAD_DATA(cx)->gcFreeLists.doubles) {
...
                js_NewDoubleInRootedValue(cx, d, &v);
            JS_ASSERT(ok);
            return true;
        }
        return E::handleDoubleOOM(cx, d, v);

This implies that when E is FailDoubleOOMHandler an empty double free list will be treated as OOM error. This is not correct since an empty free list just means that a particular GC arena is exhausted, not that it is impossible to find or allocate more GC arenas.

Comment 1

[David Mandelin [:dmandelin]]

The original reason for that code was to create a boxing function |js_NativeToValue| that can be called from trace. At that time, it was not possible to allocate doubles while on trace, so it had to simply fail. If this is no longer the case, then allowing |js_NativeToValue| to handle this case makes sense.

Comment 2

[Igor Bukanov]

Attachment: fix v1

The fix avoids OOM reporting in js_NewDoubleInRooted but delegates that to FailDoubleOOMHandler. That is still not good as it would report OOM even if the GC heap is full of garbage. But at least it fixes the bug in question delegating the full solution until we can run the GC from the trace.

Comment 3

[Igor Bukanov]

(In reply to comment #1)
> The original reason for that code was to create a boxing function
> |js_NativeToValue| that can be called from trace. At that time, it was not
> possible to allocate doubles while on trace, so it had to simply fail. If this
> is no longer the case, then allowing |js_NativeToValue| to handle this case
> makes sense.

The code before changes from bug 504797 cannot fail since it uses the reserve pool. Now, that was suboptimal since it would use the pool on the empty pool condition. But at least that was just an optimization bug, not a bogus oom.

Comment 4

[Igor Bukanov]

Attachment: fix v2

Here is the right patch without typos.

Comment 5

[David Mandelin [:dmandelin]]

Comment on attachment 407550 [details] [diff] [review]
fix v2

This bug confused me at first but after reading the patch I think I understand. r+ assuming the following makes sense (if not, then apparently I don't know what I'm talking about):

As of now, we have never been able to run an GC on trace. The old code tried to protect itself against that by calling js_NewDoubleInRootedValue only if that would not cause a GC (thus embedding details of allocator operation in NativeToValue). But at least now, js_NewDoubleInRootedValue (actually RefillDoubleFreeList) will refuse to do a GC if on trace. So the trace code can just call js_NewDoubleInRootedValue, and report OOM if it fails. We still have 2 trait classes because the recorder can use the reserved pool if it wants. This new scheme is simpler, will report a (false) OOM less, and doesn't depend on allocator implementation details.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> As of now, we have never been able to run an GC on trace. The old code tried to
> protect itself against that by calling js_NewDoubleInRootedValue only if that
> would not cause a GC (thus embedding details of allocator operation in
> NativeToValue). But at least now, js_NewDoubleInRootedValue (actually
> RefillDoubleFreeList) will refuse to do a GC if on trace. So the trace code can
> just call js_NewDoubleInRootedValue, and report OOM if it fails.

That is a nice summary.

Comment 7

[Igor Bukanov]

(In reply to comment #5)
> This new scheme is simpler, will report a (false) OOM less, and doesn't depend
> on allocator implementation details.

Note that currently even with the disabled JIT a bogus OOM report can still happen. When malloc returns null it does not indicate that there is no memory since we do not attempt to run a GC cycle and try malloc again to see if any memory was reclaimed. Such last-ditch GC would almost certainly introduce a lot of GC hazards due to assumptions in the code that malloc never runs a GC.

Now, with the no-GC-under-the-trace policy the situation is slightly worse than OOM-reporting in JSContext::malloc. The allocator currently refuses to allocate beyond JSRuntime::maxGCBytes. So with the last-ditch GC disabled we will report OOM when the GC simply hits that limit when in theory we could try to use malloc to allocate more GC arenas. 

This suggests to ignore gcMaxBytes limit when on trace. This will make the traced code on pair with the rest of code dealing with the failed malloc.

Comment 8

[Igor Bukanov]

I filed a (In reply to comment #7)
> This suggests to ignore gcMaxBytes limit when on trace.

I filed bug 523701 about this.

Comment 9

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/a389613bd931

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This landed on 1.9.2 already? The flags seem to be marked as such.

Comment 11

[Robert Sayre]

no, this didn't land

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like those flags were set while this bug has been cloned.

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a389613bd931