572063 - remove AutoObjectRooter, AutoIdRooter, AutoValueRooter

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

No longer needed. Most calls to AutoArrayRooter aren't needed either (except one).

I get a couple failures. Probably a typo. Will look.

REGRESSIONS
    js1_5/GC/regress-390078.js
    js1_5/Regress/regress-203278-1.js
    js1_6/extensions/regress-475144.js
    js1_5/Array/regress-157652.js
    js1_5/Array/regress-330812.js
    js1_7/extensions/regress-455982-01.js
    js1_7/extensions/regress-455982-02.js
    js1_7/regress/regress-469239-01.js
    js1_8/extensions/regress-452476.js
    js1_8/regress/regress-466787.js
    js1_8/regress/regress-477581.js
    js1_8_5/regress/regress-555246-1.js
    js1_8_1/trace/trace-test.js
    js1_5/Regress/regress-422348.js

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Easy fix. Typo. I used *objp instead of obj in ObjectToIterator in jstracer.cpp.

Comment 3

[Andreas Gal :gal]

Passes all tests, crashes SS (during GC). This points to a lack of GC coverage in our test suite. We should probably will file a separate bug on that. I will look into the crash.

Comment 4

[Andreas Gal :gal]

(gdb) b js_GC 
Breakpoint 1 at 0x1000507c1: file ../jsgc.cpp, line 3542.
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /Users/gal/workspace/tracemonkey-repository/js/src/Darwin_OPT.OBJ/js 
Reading symbols for shared libraries . done
js> var d = Date.now(); load("t/3d-cube.js"); load("t/3d-morph.js"); load("t/3d-raytrace.js"); load("t/access-binary-trees.js"); load("t/access-fannkuch.js"); load("t/access-nbody.js"); load("t/access-nsieve.js"); load("t/bitops-3bit-bits-in-byte.js"); load("t/bitops-bits-in-byte.js"); load("t/bitops-bitwise-and.js"); load("t/bitops-nsieve-bits.js"); load("t/controlflow-recursive.js"); load("t/crypto-aes.js"); load("t/crypto-md5.js"); load("t/crypto-sha1.js"); load("t/date-format-tofte.js"); load("t/date-format-xparb.js"); load("t/math-cordic.js"); load("t/math-partial-sums.js"); load("t/math-spectral-norm.js"); load("t/regexp-dna.js"); load("t/string-base64.js"); load("t/string-fasta.js"); load("t/string-tagcloud.js"); load("t/string-unpack-code.js"); load("t/string-validate-input.js"); print(Date.now() - d);

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: 13 at address: 0x0000000000000000
0x00000001000e1a09 in JSObject::setProperty () at /Users/gal/workspace/tracemonkey-repository/js/src/jsobj.h:1607
1607	    return !!arrayobj->setProperty(cx, INT_TO_JSID(count), &v);
(gdb) 
(gdb) bt
#0  0x00000001000e1a09 in JSObject::setProperty () at /Users/gal/workspace/tracemonkey-repository/js/src/jsobj.h:1607
#1  0x00000001000e1a09 in MatchCallback (cx=0x100311fd0, count=0, p=<value temporarily unavailable, due to optimizations>) at ../jsstr.cpp:1607
#2  0x00000001000dfa19 in DoMatch (cx=0x100311fd0, vp=0x1004003b8, str=0x10567b1c0, g=@0x7fff5fbfed80, callback=0x1000e1970 <MatchCallback(JSContext*, unsigned long, void*)>, data=0x7fff5fbfedd8, flags=<value temporarily unavailable, due to optimizations>) at ../jsstr.cpp:1607
#3  0x00000001000e2dd4 in str_match (cx=0x100311fd0, argc=1, vp=0x1004003b8) at ../jsstr.cpp:1607
#4  0x000000010005d89c in js_Interpret (cx=0x100311fd0) at jsops.cpp:1607
#5  0x0000000100064bbc in js_Execute (cx=0x100311fd0, chain=0x101202000, script=<value temporarily unavailable, due to optimizations>, down=<value temporarily unavailable, due to optimizations>, flags=2, result=0x0) at jsinterp.cpp:1607
#6  0x000000010000efb0 in JS_ExecuteScript (cx=0x100311fd0, obj=<value temporarily unavailable, due to optimizations>, script=<value temporarily unavailable, due to optimizations>, rval=<value temporarily unavailable, due to optimizations>) at ../jsapi.cpp:1607
#7  0x0000000100004c51 in Load (cx=0x100311fd0, obj=0x101202000, argc=1, argv=<value temporarily unavailable, due to optimizations>, rval=<value temporarily unavailable, due to optimizations>) at ../../shell/js.cpp:1607
#8  0x0000000100065430 in js_Invoke (cx=0x100311fd0, args=@0x7fff5fbff570, flags=2) at jsinterp.cpp:1607
#9  0x00000001000547d6 in js_Interpret (cx=0x100311fd0) at jsops.cpp:1607
#10 0x0000000100064bbc in js_Execute (cx=0x100311fd0, chain=0x101202000, script=<value temporarily unavailable, due to optimizations>, down=<value temporarily unavailable, due to optimizations>, flags=2, result=0x7fff5fbff7c8) at jsinterp.cpp:1607
#11 0x000000010000efb0 in JS_ExecuteScript (cx=0x100311fd0, obj=<value temporarily unavailable, due to optimizations>, script=<value temporarily unavailable, due to optimizations>, rval=<value temporarily unavailable, due to optimizations>) at ../jsapi.cpp:1607
#12 0x0000000100003e07 in Process (cx=0x100311fd0, obj=0x101202000, filename=<value temporarily unavailable, due to optimizations>, forceTTY=<value temporarily unavailable, due to optimizations>) at ../../shell/js.cpp:1607
#13 0x0000000100007139 in shell (cx=0x100311fd0, argc=0, argv=0x7fff5fbff958, envp=0x7fff5fbff960) at ../../shell/js.cpp:1607
#14 0x0000000100007516 in main (argc=1, argv=0x7fff5fbff950, envp=0x7fff5fbff960) at ../../shell/js.cpp:1607

We die with jit off in regexp code. Has to be another typo. No gc occurred yet.

Comment 5

[Igor Bukanov]

As a first step we should keep the classes and their usage but replace the implementation with asserts that the things are marked.

Comment 6

[Andreas Gal :gal]

Attachment: patch

Easy fix. Another typo.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Does this mean we can nix the nsContentUtils auto rooters too?

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> Does this mean we can nix the nsContentUtils auto rooters too?

It looks the answer is yes. The only potential source of troubles can comefrom a pattern like

nsAutoGCRoot root();

{
   JSAutoRequest r1;
   store_val_in_root;
}
...
{
   JSAutoRequest r2;
   get_val_in_root;
}

With conservative GC the above pattern would be unsafe since the GC does not scan the stack if thread is not in a request. But there is no such usage AFAICS.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Still a valid non-TM bug?

Comment 10

[Igor Bukanov]

(In reply to Ryan VanderMeulen from comment #9)
> Still a valid non-TM bug?

Yes.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We're converging on a plan to remove the conservative scanner. Brian has a static analysis for finding rooting bugs. It will be a lot of work to do this, but I think it's worth it.

It's not entirely clear what sort of rooting API we'll use in the future, but I think it will be easier to transform the old API into the new one rather than removing the old one and then putting in a new one.

Comment 12

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #11)
> We're converging on a plan to remove the conservative scanner. Brian has a
> static analysis for finding rooting bugs. It will be a lot of work to do
> this, but I think it's worth it.

I second that. It became rather clear for me that the conservative GC put too much requirements on an allocator and prevents many optimizations.

Comment 13

[Andreas Gal :gal]

As long it's an analysis at every checkin static analysis is strictly superior to conservative scanning. We should use it to infer a minimum set of rooting operations though instead of using these ugly and verbose manual rooting constructs.