Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 14 years ago

__defineSetter__("x", Array.reduce)
x = Proxy.create(function() {},
this.watch("x",
function() {
  yield
}))

asserts js debug shell on TM tip without -j at Assertion failure: pc >= script->main && pc < script->code + script->length, at ../jsopcode.cpp:5142

Assuming related to harmony:proxies.

	Andreas Gal :gal
	Comment 1 • 14 years ago

#0  0x0000000100153c92 in JS_Assert (s=0x1001ee1f0 "pc >= script->main && pc < script->code + script->length", file=0x1001ecd93 "../jsopcode.cpp", ln=5142) at ../jsutil.cpp:77
#1  0x00000001000e5258 in js_DecompileValueGenerator (cx=0x10083c800, spindex=1, v=4315967088, fallback=0x101405900) at ../jsopcode.cpp:5142
#2  0x00000001000389f5 in js_ReportMissingArg (cx=0x10083c800, vp=0x101000308, arg=0) at ../jscntxt.cpp:2105
#3  0x000000010002ce8d in array_extra (cx=0x10083c800, mode=REDUCE, argc=0, vp=0x101000308) at ../jsarray.cpp:2824
#4  0x000000010002d6e1 in array_reduce (cx=0x10083c800, argc=0, vp=0x101000308) at ../jsarray.cpp:3011
#5  0x00000001000140af in js_generic_fast_native_method_dispatcher (cx=0x10083c800, argc=0, vp=0x101000308) at ../jsapi.cpp:4309
#6  0x00000001000ae245 in js_Invoke (cx=0x10083c800, args=@0x7fff5fbfe560, flags=0) at jsinterp.cpp:531
#7  0x00000001000aed84 in js_InternalInvoke (cx=0x10083c800, obj=0x101402000, fval=4315967088, flags=0, argc=1, argv=0x101000188, rval=0x101000188) at jsinterp.cpp:678
#8  0x0000000100048119 in js_watch_set (cx=0x10083c800, obj=0x101402000, id=4297483604, vp=0x101000188) at ../jsdbgapi.cpp:712
#9  0x00000001000482ad in js_watch_set_wrapper (cx=0x10083c800, obj=0x101402000, argc=1, argv=0x101000140, rval=0x101000188) at ../jsdbgapi.cpp:738
#10 0x00000001000ae7d2 in js_Invoke (cx=0x10083c800, args=@0x7fff5fbfe920, flags=2) at jsinterp.cpp:639
#11 0x00000001000aed84 in js_InternalInvoke (cx=0x10083c800, obj=0x101402000, fval=4315969056, flags=0, argc=1, argv=0x7fff5fbff008, rval=0x7fff5fbff008) at jsinterp.cpp:678
#12 0x00000001000aeeca in js_InternalGetOrSet (cx=0x10083c800, obj=0x101402000, id=4297483604, fval=4315969056, mode=JSACC_WRITE, argc=1, argv=0x7fff5fbff008, rval=0x7fff5fbff008) at jsinterp.cpp:714
#13 0x00000001000cdd0d in JSScopeProperty::set (this=0x1008a2b70, cx=0x10083c800, obj=0x101402000, vp=0x7fff5fbff008) at jsscope.h:1004
#14 0x00000001000bf5cd in js_NativeSet (cx=0x10083c800, obj=0x101402000, sprop=0x1008a2b70, added=false, vp=0x7fff5fbff008) at ../jsobj.cpp:4799
#15 0x00000001000c2653 in js_SetPropertyHelper (cx=0x10083c800, obj=0x101402000, id=4297483604, defineHow=9, vp=0x7fff5fbff008) at ../jsobj.cpp:5206
#16 0x00000001000990de in js_Interpret (cx=0x10083c800) at jsops.cpp:1825
#17 0x00000001000ad906 in js_Execute (cx=0x10083c800, chain=0x101402000, script=0x1004147e0, down=0x0, flags=0, result=0x0) at jsinterp.cpp:837
#18 0x0000000100011c2f in JS_ExecuteScript (cx=0x10083c800, obj=0x101402000, script=0x1004147e0, rval=0x0) at ../jsapi.cpp:4832
#19 0x00000001000099e0 in Process (cx=0x10083c800, obj=0x101402000, filename=0x7fff5fbffa95 "x.js", forceTTY=0) at ../../shell/js.cpp:422
#20 0x000000010000a625 in ProcessArgs (cx=0x10083c800, obj=0x101402000, argv=0x7fff5fbff948, argc=1) at ../../shell/js.cpp:836
#21 0x000000010000a7a0 in main (argc=1, argv=0x7fff5fbff948, envp=0x7fff5fbff958) at ../../shell/js.cpp:5084

	Andreas Gal :gal
	Comment 2 • 14 years ago

My money is on an existing bug in watch involving non-native objects. I am going to hide this until we know whats going on since if I am correct liveconnect can trigger this, too.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 3 • 14 years ago

I think [sg:investigate] has been deprecated - assume [sg:critical?] unless otherwise.

	Igor Bukanov Assignee
	Comment 4 • 14 years ago

(In reply to comment #3)
> I think [sg:investigate] has been deprecated - assume [sg:critical?] unless
> otherwise.

There is a bug there, but it became GC hazard only with proxies. I will file a bug on it shortly.

	Igor Bukanov Assignee
	Comment 5 • 14 years ago

(In reply to comment #4)
> (In reply to comment #3)
> > I think [sg:investigate] has been deprecated - assume [sg:critical?] unless
> > otherwise.
> 
> There is a bug there, but it became GC hazard only with proxies. I will file a
> bug on it shortly.

I meant separated bug.

	Brendan Eich [:brendan]
	Comment 6 • 14 years ago

js> __defineSetter__("x", Array.reduce)
js> x = Proxy.create(function() {},
this.watch("x",
function() {
  yield
}))
typein:4: TypeError: missing argument 0 when calling function reduce
js> 

in opt shell -- Gary, can you confirm no problem in opt under valgrind (not proof of no bug, but worth noting).

/be

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 7 • 14 years ago

$ cat testcase.js | valgrind ./js-opt-32-tm-linux -j -i
==8885== Memcheck, a memory error detector
==8885== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==8885== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==8885== Command: ./js-opt-32-tm-linux -j -i
==8885== 
js> __defineSetter__("x", Array.reduce)
js> x = Proxy.create(function() {},
this.watch("x",
function() {
  yield
}))
typein:4: TypeError: missing argument 0 when calling function reduce
js> 
js> 
==8885== 
==8885== HEAP SUMMARY:
==8885==     in use at exit: 109 bytes in 6 blocks
==8885==   total heap usage: 995 allocs, 989 frees, 1,446,295 bytes allocated
==8885== 
==8885== LEAK SUMMARY:
==8885==    definitely lost: 0 bytes in 0 blocks
==8885==    indirectly lost: 0 bytes in 0 blocks
==8885==      possibly lost: 0 bytes in 0 blocks
==8885==    still reachable: 109 bytes in 6 blocks
==8885==         suppressed: 0 bytes in 0 blocks
==8885== Rerun with --leak-check=full to see details of leaked memory
==8885== 
==8885== For counts of detected and suppressed errors, rerun with: -v
==8885== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 23 from 8)

	Andreas Gal :gal
	Comment 8 • 14 years ago

This has nothing to do with proxies. Looks like the generator function leaves a frame on the stack where pc points to JSOP_GENERATOR, which the assert disallows (main is after JSOP_GENERATOR).

__defineSetter__("x", Array.reduce)
this.watch("x", function() { yield });
x = 5

	Igor Bukanov Assignee
	Comment 9 • 14 years ago

The reason for the bug is that js_watch_set uses script->code, not script->main, for pseudoframe's PC that it constructs to invoke the watchpoint setter. With that setup eventually the decompiler tries to use that frame for error reporting and hits the assert as the decompiler assumes that it cannot be invoked from a script prologue.

	Igor Bukanov Assignee
	Comment 10 • 14 years ago

The bug is not critical thanks to the use of the local asserts in the decompiler that do the checks and early quit even in optimized builds. Hence at worst the bug would lead to a script termination in the same as it would happen when the browser terminates slow-running scripts.

	Brendan Eich [:brendan]
	Comment 11 • 14 years ago

So unrestrict the bug? Reviewing.

/be

	Brendan Eich [:brendan]
	Comment 12 • 14 years ago

Comment on attachment 448561 [details] [diff] [review]
v1

Why is script->main the right thing here? Or is it a matter of indifference except to the decompiler, and except for that local assert?

What I'm asking is really why wouldn't the fake frame want its pc to be at the script prolog, in theory?

/be

	Igor Bukanov Assignee
	Comment 13 • 14 years ago

(In reply to comment #12)
> Why is script->main the right thing here? Or is it a matter of indifference
> except to the decompiler, and except for that local assert?

That is a very good point. IIRC the decompiler has never bothered with modelling of the stack for function prologue leading to the restriction expressed by bug's assert. So the fix is the right for the decompiler. But in general the fix would mean that a generator function would exist on the stack frame which pc points after JSOP_GENERATOR. This could be harmless, but I am not sure about it.

	Brendan Eich [:brendan]
	Comment 14 • 14 years ago

Can we fix the decompiler then?

/be

	Igor Bukanov Assignee
	Comment 15 • 14 years ago

Comment on attachment 448561 [details] [diff] [review]
v1

I will fix the decompiler directly after checking one more time that having a generator function on the stack even if its PC points at JSOP_GENERATOR is ok.

	Daniel Veditz [:dveditz]
	Comment 16 • 14 years ago

Not blocking but we wouldn't say "no" to a patch. Marking not a security exploit, please put it back to critical if I've misinterpreted the discussion. If I got that right then we should be able to unhide the bug, correct?

	Igor Bukanov Assignee
	Comment 17 • 14 years ago

Lets keep this closed until Tuesday next week. Although the bug in question is harmless, I am not sure that having a generator frame on the stack is fully safe and I need few  days to check things.

	Igor Bukanov Assignee
	Comment 18 • 14 years ago

AFAICS there is no security implications of this bug.

	Jesse Ruderman
	Comment 19 • 14 years ago

WFM

The first good revision is:
changeset:   50898:b22e82ce2364
user:        Luke Wagner <lw@mozilla.com>
date:        Thu Aug 19 18:02:17 2010 -0700
summary:     Bug 589015 - js_watch_set doesn't need that crazy dummy frame (r=mrbkap)

	Christian Holler (:decoder)
	Comment 20 • 12 years ago

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929