991036 - Improve temperamental assertion in js::jit::Label::~Label()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

~Label() {
        if (MaybeGetIonContext())
            JS_ASSERT_IF(!GetIonContext()->runtime->hadOutOfMemory(), !used());
    }

This assertion fails a lot in oom testing because runtime->hadOutOfMemory is only set when js_ReportOutOfMemory is called, not every time something like js_malloc fails.

The easiest way to fix it is to delete the assertion.

This patch instead disables the assertion when if the state of the OOM testing machinery indicates that at least one allocation has failed. It would still trip when OOM happens naturally, and since there's a data race on OOM_counter, it could just randomly fail. Hoping that will be rare enough that we don't care.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: bug-991036-Label-assertion-v1.patch

Comment 2

[Nicolas B. Pierron [:nbp]]

I would prefer if we can find a solution to get this assertion right.

This assertion is here to prevent dangling pointers in the generated code, but we do not care about dangling pointers if the code is going to be discarded (such as in case of OOMs).

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8400578 [details] [diff] [review]
bug-991036-Label-assertion-v1.patch

Review of attachment 8400578 [details] [diff] [review]:
-----------------------------------------------------------------

This assert has caused a lot of trouble. With this patch we should still catch problems in non-OOM situations, good.

::: js/src/jit/shared/Assembler-shared.h
@@ +361,5 @@
> +        if (OOM_counter > OOM_maxAllocations)
> +            return;
> +#endif
> +        if (MaybeGetIonContext() && GetIonContext()->runtime->hadOutOfMemory())
> +            return;

Pre-existing, but please wrap this in an #ifdef DEBUG while you're here: GetIonContext() and friends do a TLS lookup and can be relatively slow with NSPR, see bug 937287.

Comment 4

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4aa2555326

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d4aa2555326