The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla31

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8400578 [details] [diff] [review]
bug-991036-Label-assertion-v1.patch
Assignee: general → jorendorff
Attachment #8400578 - Flags: review?(jdemooij)
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).
(Assignee)

Updated

3 years ago
Blocks: 912928, 988953
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.
Attachment #8400578 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4aa2555326
Summary: Fix broken assertion in js::jit::Label::~Label() → Improve temperamental assertion in js::jit::Label::~Label()
https://hg.mozilla.org/mozilla-central/rev/6d4aa2555326
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.