551705 - TM: (32-bit) Crash [@ JSString::hasFlag] or (32-bit) [@ js_ConcatStrings] or (64-bit) "Assertion failure: !JSVAL_IS_PRIMITIVE(v), at ../jsnum.cpp" or (64-bit) [@ js_ValueToNumber]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(Function("\
  for each(let x in [\
  true,\
  (1),\
  (1),\
  (1),\
  (1),\
  true,\
  true,\
  true,\
  (1),\
  true,\
  true,\
  (1),\
  true,\
  true,\
  (1),\
  (1),\
  true,\
  true,\
  true,\
  true\
  ]) { \
    ((function f(aaaaaa) {\
       return aaaaaa.length == 0 ? 0 : aaaaaa[0] + f(aaaaaa.slice(1))\
     })([\
      x,\
      Math.I,\
      '',\
      null,\
      Math.I,\
      null,\
      new String(),\
      new String()\
      ]))\
}"))()

crashes debug JS shell at JSString::hasFlag near null on TM tip with -j and crashes opt shell on TM tip with -j at js_ConcatStrings near null. (Both 32-bit shells)

For 64 bits on both Mac 10.6.2 and Ubuntu, it asserts debug at Assertion failure: !JSVAL_IS_PRIMITIVE(v), at ../jsnum.cpp:985 and crashes opt at js_ValueToNumber near null.

Security-sensitive due to memory address being on stack - I don't know if the patch that caused this has landed on m-c or not.

===

Opt shell stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x000000000000000a
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   js-opt-32-tm-darwin           	0x000ce052 js_ConcatStrings + 18
1   ???                           	0x001fb90b 0 + 2078987
2   js-opt-32-tm-darwin           	0x0010f79c js::ExecuteTree(JSContext*, js::TreeFragment*, unsigned int&, js::VMSideExit**) + 732
3   js-opt-32-tm-darwin           	0x0012b838 js::MonitorLoopEdge(JSContext*, unsigned int&, js::RecordReason) + 1048
4   js-opt-32-tm-darwin           	0x0005b1bf js_Interpret + 43039
5   js-opt-32-tm-darwin           	0x00060b91 js_Execute + 625
6   js-opt-32-tm-darwin           	0x0000e0fc JS_ExecuteScript + 60
7   js-opt-32-tm-darwin           	0x00004e58 Process(JSContext*, JSObject*, char*, int) + 1336
8   js-opt-32-tm-darwin           	0x00008f16 main + 1734
9   js-opt-32-tm-darwin           	0x000029ed _start + 208
10  js-opt-32-tm-darwin           	0x0000291c start + 40

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to bug 530900:

The first bad revision is:
changeset:   38563:12827fc411c1
user:        David Anderson
date:        Mon Mar 08 10:28:08 2010 -0800
summary:     Trace recursion when the return keyword is omitted (bug 530900, r=gal).

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Changing this code in the linked bug was not necessary, and in fact wrong. It was trying to make a very tiny, irrelevant optimization - if we just popped a frame that had a JSOP_STOP instead of a JSOP_RETURN, just write back a constant. But by this point we've lost the return pc, and reading from the current pc just gets us a JSOP_TRACE that's after the JSOP_CALL.

So the check fails and it takes the wrong branch. This optimization is pointless so I've just reverted the code.

Comment 3

[Andreas Gal :gal]

Comment on attachment 432021 [details] [diff] [review]
fix

offset = sp_adj - sizeof(double) maybe?

Comment 4

[Daniel Veditz [:dveditz]]

bug 530900 was backed out on the branch so this issue is tracemonkey branch only.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/1f812d89de66

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1f812d89de66

Comment 7

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug551705.js.