450529 - TM: asserts on foxnews.com in 3rd party script

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: trace + gdb dump

Script looks third party, "Prototype JavaScript framework, version 1.5.1.2"

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

That assertion doesn't like that regs->pc is out of date, and it wants to peek ahead in the bytecode stream.  We could probably parameterize match_or_replace to have the caller pass in the implied-test bit, since we can tell at record time that we hit that case.

Comment 2

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

FWIW, seeing this on more than one site -- same assert on Apple's iPhone apps store.

Comment 3

[Andreas Gal :gal]

Builtins shouldn't reach back into the interpreter. We might want to set cx->fp to NULL when entering a trace, and back to its original value on exit in debug builds to catch these cases.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Many thanks to Brendan and Andreas for holding my hand through the builtin jungle.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 333877 [details] [diff] [review]
Proposed fix

>+js_str_match_helper(JSContext *cx, uintN argc, jsval *vp, jsbytecode *pc)

Been using js_StringMatchHelper and the like (see js_StringReplaceHelper).

I'm hip to pc at end.

> TraceRecorder::record_JSOP_CALL()
> {
>-    uintN argc = GET_ARGC(cx->fp->regs->pc);
>+    uintN argc = GET_ARGC(pc);

We talked, you're cool with a local pc.

Need a trace-tests.js addition to cover this case. Thanks,

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/index.cgi/rev/3eaa6a428d52 includes the trace-tests.js testcase.

Comment 7

[Bob Clary [:bc] (inactive)]

in js1_8_1/trace/trace-test.js also