482015 - TM: Assertion failure: cx->bailExit due to _RETRY builtins that call JS_malloc

Status: RESOLVED DUPLICATE of bug 487216

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

Currently traceable variants of str_substring is declared as STRING_RETRY:

JS_DEFINE_TRCINFO_2(str_substring,
    (4, (static, STRING_RETRY,      String_p_substring, CONTEXT, THIS_STRING, INT32, INT32,   1, 1)),
    (3, (static, STRING_RETRY,      String_p_substring_1, CONTEXT, THIS_STRING, INT32,        1, 1)))
J

But this is wrong as the substring implementation reports OOM. The reason for that is that js_NewDependentString must call JS_malloc when the length of the substring exceeds 32K, the maximum value for the dependent string length. The following demonstrates this in js shell:

@watson~> cat ~/s/x1.js
function f()
{
    var s = Array(5<<20).join(".");
    var a = [];
    for (var i = 0; i != 500; ++i) {
        a.push(s.substring(1));
    }
}

f();
@watson~> ~/build/js32.tm.dbg/js -j ~/s/x1.js
Assertion failure: cx->bailExit, at /home/igor/m/tm/js/src/jstracer.cpp:4709
Trace/breakpoint trap

Note that to run the example within a reasonable time limit one either needs a system where the amount of physical RAM exceeds the address space for 32-bit process or use hacks like ulimit -v 500000.

Comment 1

[Andreas Gal :gal]

Could you post the bt stack dump for this? I couldn't reproduce it.

Comment 2

[Igor Bukanov]

> Could you post the bt stack dump for this? I couldn't reproduce it.

Does the example finishes with out-of-memory report in your case? Here is a stack with the current TM tip, 5374efa7d144 :

#1  0x08150f73 in js_DeepBail (cx=0x823fec8) at /home/igor/m/tm/js/src/jstracer.cpp:4709
#2  0x08079f70 in js_LeaveTrace (cx=0x823fec8) at /home/igor/m/tm/js/src/jscntxt.h:1404
#3  0x08079f87 in js_GetTopStackFrame (cx=0x823fec8) at /home/igor/m/tm/js/src/jscntxt.h:1428
#4  0x0807b430 in PopulateReportBlame (cx=0x823fec8, report=0xff84689c) at /home/igor/m/tm/js/src/jscntxt.cpp:1003
#5  0x0807b518 in js_ReportOutOfMemory (cx=0x823fec8) at /home/igor/m/tm/js/src/jscntxt.cpp:1034
#6  0x0805724b in JS_ReportOutOfMemory (cx=0x823fec8) at /home/igor/m/tm/js/src/jsapi.cpp:5684
#7  0x08060fe2 in JS_malloc (cx=0x823fec8, nbytes=10485758) at /home/igor/m/tm/js/src/jsapi.cpp:1873
#8  0x0811e687 in js_NewStringCopyN (cx=0x823fec8, s=0xf6f5100a, n=5242878) at /home/igor/m/tm/js/src/jsstr.cpp:2959
#9  0x0811eb5b in js_NewDependentString (cx=0x823fec8, base=0x8241698, start=1, length=5242878) at /home/igor/m/tm/js/src/jsstr.cpp:2904
#10 0x0811ee0f in SubstringTail (cx=0x823fec8, str=0x8241698, length=5242879, begin=1, end=5242879) at /home/igor/m/tm/js/src/jsstr.cpp:767
#11 0x0811ef20 in String_p_substring_1 (cx=0x823fec8, str=0x8241698, begin=1) at /home/igor/m/tm/js/src/jsstr.cpp:827
#12 0xf7bd4f63 in ?? ()
#13 0xff8490a8 in ?? ()
#14 0x08176363 in js_MonitorLoopEdge (cx=0x823fec8, inlineCallCount=@0xff849744) at /home/igor/m/tm/js/src/jstracer.cpp:4308
#15 0x081c2735 in js_Interpret (cx=0x823fec8) at /home/igor/m/tm/js/src/jsinterp.cpp:3667
#16 0x080b477a in js_Execute (cx=0x823fec8, chain=0x8243000, script=0x824ab00, down=0x0, flags=0, result=0x0) at /home/igor/m/tm/js/src/jsinterp.cpp:1561
#17 0x0805a11e in JS_ExecuteScript (cx=0x823fec8, obj=0x8243000, script=0x824ab00, rval=0x0) at /home/igor/m/tm/js/src/jsapi.cpp:5015
#18 0x08052a61 in Process (cx=0x823fec8, obj=0x8243000, filename=0xff84ab6a "/home/igor/s/x1.js", forceTTY=0) at /home/igor/m/tm/js/src/shell/js.cpp:388
#19 0x080535ed in ProcessArgs (cx=0x823fec8, obj=0x8243000, argv=0xff849a58, argc=2) at /home/igor/m/tm/js/src/shell/js.cpp:782
#20 0x080538c1 in main (argc=2, argv=0xff849a58, envp=0xff849a64) at /home/igor/m/tm/js/src/shell/js.cpp:4634

Comment 3

[Jason Orendorff [:jorendorff]]

Ideally _RETRY functions, if they end up doing js_ReportOutOfMemory or js_ReportAllocationOverflow, should just return FALSE without reporting any error or setting any exception pending. We should fall off trace and retry from the interpreter. (If the error condition happens again then, fine; if it doesn't, fine.)

Igor, can we make these two functions behave that way?  They can determine whether they have been called (indirectly) from a _RETRY function by testing `JS_ON_TRACE(cx) && !cx->bailExit`.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Igor, can we make these two functions behave that way? 

The error report comes from JS_malloc which is 4 calls bellow on the native stack than the string functions in question. So the question is should JS_malloc silently report null if it is called from a _RETRY function?

Comment 5

[Andreas Gal :gal]

Jason, is !cx->bailExit guaranteed in OPT builds when not inside a _FAIL builtin?

Comment 6

[Jason Orendorff [:jorendorff]]

> Jason, is !cx->bailExit guaranteed in OPT builds when not inside a
> _FAIL builtin?

No it's not! Good catch. The _RETRY methods that might lead to malloc calls could explicitly set cx->bailExit = NULL first. (Or we could just remove the #ifdef DEBUG around the code that clears it after each _FAIL builtin call. But that has a performance cost--maybe negligible.)

> So the question is should JS_malloc silently report null if it is
> called from a _RETRY function?

Well, if we want to have _RETRY functions that call JS_malloc (directly or indirectly), this is the only possible correct behavior, right?

Looking over the _RETRY functions, I think most of them probably do currently call JS_malloc in some cases. It seems like the alternative is to convert those to _FAIL functions.

Comment 7

[Andreas Gal :gal]

We should try -> _FAIL first. Maybe there is no perf overhead. If its slower we can tr setting cx->bailExit to NULL and see how that compares.

Comment 8

[Jason Orendorff [:jorendorff]]

A problem with just switching these functions to _FAIL is that a lot of the builtins we call directly via lir->insCall(), which cannot be _FAIL, can end up in js_ReportOutOfMemory.  js_ConcatStrings, for example.

We could emit `cx->bailExit = NULL` before each of those.

Comment 9

[Andreas Gal :gal]

Why can't we set bailExit to NULL inside the actual builtins that might run into this state?

Comment 10

[Jason Orendorff [:jorendorff]]

We can do that.  I think what you're suggesting is like the patch in bug 482958, plus changes to fix the direct insCall()s by setting cx->bailExit to NULL.  Could you look at that patch and weigh in there?

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 12

[Jason Orendorff [:jorendorff]]

That patch punts on the constructors, which gal will fix in bug 487134.

Comment 13

[Jason Orendorff [:jorendorff]]

I fixed a bug in my modified version of String_p_charAt.

http://hg.mozilla.org/tracemonkey/rev/e201de53e918

Comment 14

[Jason Orendorff [:jorendorff]]

Btw, that patch does not address comment 8.  But it is an improvement.  Filed followup bug 487216 for the rest, patch coming there...

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e201de53e918