566949 - JSContext::malloc should not insist on a call outside a request

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

JSContext::malloc or JS_malloc is allowed to be called outside a request, yet the TriggerGC call that it does insists on that. That could a reason for a test case failure like in bug 237006 comment 6.

Comment 1

[Igor Bukanov]

Attachment: v1

Here is an essentially one-liner to disable js_TriggerGC calls outside request on malloc pressure.

Comment 2

[Igor Bukanov]

Attachment: v1 for real

v1 is unrelated patch, sorry for bugzilla spam

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 446495 [details] [diff] [review]
v1 for real

>-    if (runtime->isGCMallocLimitReached())
>+
>+    /* Trigger the GC on memory pressure but only if we are inside a request. */
>+    if (runtime->isGCMallocLimitReached() &&
>+        (requestDepth != 0 || thread == runtime->gcThread))

We shouldn't be malloc-ing during GC, so why allow
`thread == runtime->gcThread` here?

js_TriggerGC asserts that !rt->gcRunning, so if we ever *did* get here during GC, I think it would be a bug.

r=me with that sorted out.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> (From update of attachment 446495 [details] [diff] [review])
> >-    if (runtime->isGCMallocLimitReached())
> >+
> >+    /* Trigger the GC on memory pressure but only if we are inside a request. */
> >+    if (runtime->isGCMallocLimitReached() &&
> >+        (requestDepth != 0 || thread == runtime->gcThread))

> 
Thanks for catching this, the condition should be requestDepth != 0 && thread != runtime->gcThread to avoid triggering the GC recursively.

> We shouldn't be malloc-ing during GC, so why allow
> `thread == runtime->gcThread` here?

For compatibility we should allow for JS_malloc calls as, for example, the cycle collector allocated a lot of things.

Comment 5

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/4260c0df53c0

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to comment #4)
> For compatibility we should allow for JS_malloc calls as, for example, the
> cycle collector allocated a lot of things.

Makes sense. Thanks.

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4260c0df53c0