Last Comment Bug 991036 - Improve temperamental assertion in js::jit::Label::~Label()
: Improve temperamental assertion in js::jit::Label::~Label()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla31
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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]
bug-991036-Label-assertion-v1.patch
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]
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 User image Jason Orendorff [:jorendorff] 2014-04-08 11:45:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4aa2555326
Comment 5 User image Carsten Book [:Tomcat] 2014-04-09 05:36:28 PDT
https://hg.mozilla.org/mozilla-central/rev/6d4aa2555326

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