Last Comment Bug 991036 - Improve temperamental assertion in js::jit::Label::~Label()
: Improve temperamental assertion in js::jit::Label::~Label()
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla31
Assigned To: Jason Orendorff [:jorendorff]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 912928 988953
  Show dependency treegraph
Reported: 2014-04-02 04:37 PDT by Jason Orendorff [:jorendorff]
Modified: 2014-04-09 05:36 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

bug-991036-Label-assertion-v1.patch (1.95 KB, patch)
2014-04-02 04:40 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2014-04-02 04:37:43 PDT
~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 User image Jason Orendorff [:jorendorff] 2014-04-02 04:40:26 PDT
Created attachment 8400578 [details] [diff] [review]
Comment 2 User image Nicolas B. Pierron [:nbp] 2014-04-02 04:44:07 PDT
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 User image Jan de Mooij [:jandem] 2014-04-03 08:18:05 PDT
Comment on attachment 8400578 [details] [diff] [review]

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 User image Jason Orendorff [:jorendorff] 2014-04-08 11:45:28 PDT
Comment 5 User image Carsten Book [:Tomcat] 2014-04-09 05:36:28 PDT

Note You need to log in before you can comment on or make changes to this bug.