634648 - re-enable tests disabled due to debug shell reftest assertion: !CompartmentHasLiveScripts(comp)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Tests failing on tip with debug 64b shell build with the same signature:

    e4x/decompilation/regress-429249.js
    ecma/extensions/trapflatclosure.js
    ecma_3/extensions/regress-429248.js
    js1_5/extensions/regress-422137.js
    js1_5/extensions/regress-429264.js
    js1_5/extensions/regress-431428.js
    js1_7/decompilation/regress-429252.js

(gdb) bt
#0  0x00007ffff7bcea0b in raise () from /lib/libpthread.so.0
#1  0x000000000059a2a8 in JS_Assert (s=0x729328 "!CompartmentHasLiveScripts(comp)", file=0x72930e "../js/src/jsdbgapi.cpp", ln=154)
    at ../js/src/jsutil.cpp:83
#2  0x00000000004741ed in JS_SetDebugModeForCompartment (cx=0xaefc10, comp=0xaf0150, debug=1) at ../js/src/jsdbgapi.cpp:154
#3  0x00000000004740c9 in JS_SetDebugMode (cx=0xaefc10, debug=1) at ../js/src/jsdbgapi.cpp:106
#4  0x0000000000407f49 in SetDebug (cx=0xaefc10, argc=1, vp=0x7ffff67f50a0) at ../../js/src/shell/js.cpp:1756
#5  0x00000000004d2046 in js::CallJSNative (cx=0xaefc10, native=0x407eb8 <SetDebug(JSContext*, uintN, jsval*)>, argc=1, vp=0x7ffff67f50a0)
    at ../js/src/jscntxtinlines.h:701
#6  0x00000000006eb0ee in js::Interpret (cx=0xaefc10, entryFrame=0x7ffff67f5048, inlineCallCount=0, interpMode=JSINTERP_NORMAL)
    at ../js/src/jsinterp.cpp:4763
#7  0x00000000004ce098 in js::RunScript (cx=0xaefc10, script=0xb69df0, fp=0x7ffff67f5048) at ../js/src/jsinterp.cpp:650
#8  0x00000000004cf4c1 in js::Execute (cx=0xaefc10, chain=0x7ffff6603048, script=0xb69df0, prev=0x0, flags=0, result=0x0) at ../js/src/jsinterp.cpp:1011
#9  0x000000000042de99 in JS_ExecuteScript (cx=0xaefc10, obj=0x7ffff6603048, script=0xb69df0, rval=0x0) at ../js/src/jsapi.cpp:4925
#10 0x0000000000404e9b in Process (cx=0xaefc10, obj=0x7ffff6603048, filename=0x7fffffffe4d1 "./e4x/decompilation/regress-429249.js", forceTTY=0)
    at ../../js/src/shell/js.cpp:452
#11 0x0000000000405c5c in ProcessArgs (cx=0xaefc10, obj=0x7ffff6603048, argv=0x7fffffffe160, argc=8) at ../../js/src/shell/js.cpp:868
#12 0x000000000040f883 in Shell (cx=0xaefc10, argc=8, argv=0x7fffffffe160, envp=0x7fffffffe1a8) at ../../js/src/shell/js.cpp:5531
#13 0x000000000040fa49 in main (argc=8, argv=0x7fffffffe160, envp=0x7fffffffe1a8) at ../../js/src/shell/js.cpp:5639

Comment 1

[Steve Fink [:sfink] [:s:]]

Yes, this would be me. Can I have some background? How do I run these tests, and why aren't they run automatically on pushes?

Thanks.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #1)
> How do I run these tests,

js/src/tests/jstests.py

> and why aren't they run automatically on pushes?

Make check only runs jit-tests IIRC. I have no idea why, I'd ask sayrer or somebody more familiar with the infrastructure.

Comment 3

[Brendan Eich [:brendan]]

Historically, jsreftests take too long. Waldo added some very complete ones that make them take even longer. They're worth running, but not on every push.

/be

Comment 4

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> Historically, jsreftests take too long. Waldo added some very complete ones
> that make them take even longer. They're worth running, but not on every push.

I run them locally before pushing patches that seem likely to trip jsreftest failures. They would take a long time on Tinderbox, but if you have 8 cores you can run them in 5-10 minutes locally.

Comment 5

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

Sounds like time to get some 8-core boxes for tbox.

Comment 6

[Jeff Walden [:Waldo]]

I run them locally (not in a browser), in a debug build, on a ~16mo old laptop (2 cores), in about 8 minutes.  I've occasionally run in-browser too for changes which might potentially break the browser but not the shell, and it doesn't take an inordinately long time -- order of 20mins maybe?  Been awhile since I did so.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Mark tests that need to run in debug mode

hg annotate says dmandelin gets the review.

Now that turning on debugging is asynchronous, we really ought to run tests with debug mode on if they need it (rather than making setDebug mode do something tricky to have it "on enough" for the tests.)

I slipped in a trivial error-handling fix in jstests.py because it made it harder for me to figure out how to use the script when it threw a confusing exception.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to comment #5)
> Sounds like time to get some 8-core boxes for tbox.

Seems like most of the tests are quite fast. It seems easier to make a fast test list and run that on tinderbox, no?

Comment 9

[David Mandelin [:dmandelin]]

Comment on attachment 512873 [details] [diff] [review]
Mark tests that need to run in debug mode

What does this patch do in the browser version of jstest? They use their own manifest parser, so I think this will cause errors there.

Comment 10

[Steve Fink [:sfink] [:s:]]

(In reply to comment #9)
> Comment on attachment 512873 [details] [diff] [review]
> Mark tests that need to run in debug mode
> 
> What does this patch do in the browser version of jstest? They use their own
> manifest parser, so I think this will cause errors there.

I don't understand why, but the tests pass both before and after my changes. I guess I should dig further and figure it out.

Comment 11

[Steve Fink [:sfink] [:s:]]

Oh, right. setDebug is what causes the problem, and it is not available in the browser versions, so the tests already have to explicitly check for it. They effectively skip the test if it's not there. For example:

  if (typeof trap == 'function' && typeof setDebug == 'function') {
     ...do the test...
  }

Comment 12

[Steve Fink [:sfink] [:s:]]

And to address your real question, it seems like the browser version of reftest shares the same manifest parsing code. At least, that's what seems to happen in practice, and it's sort of implied by http://bclary.com/blog/2009/09/10/browser-javascript-reftests/ although I confess I've been unable to track through to figure out what the -reftest option to the browser actually does. (The browser reftests run via runreftest.py which invokes firefox -reftest which... runs something.)

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #12)
> runreftest.py which invokes firefox -reftest which... runs something.)

That's the bit that's different. See layout/tools/reftest -- there's a parser embedded in the JS in there and there's a comment in that parser that refers to another manifest parser implementation elsewhere. (Remember this from the last time I had to look at this stuff.)

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: Mark tests that need to run in debug mode

As usual, you guys are right and I'm wrong. I was running the tests incorrectly and so it was succeeding for all the wrong reasons.

I've modified the patch to update the reftest.js parser as well. It passes along the 'debug' flag, though it is currently ignored (which is ok since the tests themselves will detect that they're not running under the shell and do nothing.) This fixes manifest parser barfs. Theoretically, we could give the browser a setDebug command that turned on debugging so that the tests would run there too, but that'll have to wait until we can turn on debugging synchronously again.

I'm running it through the try server just in case.

Comment 15

[Steve Fink [:sfink] [:s:]]

It made it through the try server, more or less. (Mochitest-other failed 4/11 runs, but each failure is different and a known orange. Similar happened with 1st try push of earlier patch. Surrounding try runs from other pushers had barely any M-oth failures. But it really does appear to be all random orange. Grr.... can't we shoot the messengers, just this once?)

Comment 16

[David Mandelin [:dmandelin]]

Comment on attachment 513040 [details] [diff] [review]
Mark tests that need to run in debug mode

Thanks for updating the parser. Bob, could you review the change to the manifest file format and jsreftest manifest parser? The intent is to run certain shell tests in debug mode, which is required to test core debugging features.

Comment 17

[Bob Clary [:bc] (inactive)]

Comment on attachment 513040 [details] [diff] [review]
Mark tests that need to run in debug mode

dbaron is reftest owner.

I'm concerned about overloading the keywords in this fashion to pass an argument to the test and in this case a single class of test (js shell) when the reftest framework is used in so many other testing areas. The choice of "debug" as a keyword seems problematic as well as it does not clearly imply the keyword is not for running tests in debug builds but for adding a specific option to the js shell to invoke the shell only function setDebug(). I would have preferred a more general approach of passing arguments rather than inventing a keyword for each possible argument that may be desired.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like you've made it a no-op in the reftest harness -- but why bother to propagate the information so far and then ignore it -- why not just drop it up-front?

That said, 'debug' also sounds like it refers to -DDEBUG, but I think you're talking about a run-time option rather than a compile-time option.  Is there a name that will make people more likely to expect that?

Comment 19

[Steve Fink [:sfink] [:s:]]

(In reply to comment #18)
> It looks like you've made it a no-op in the reftest harness -- but why bother
> to propagate the information so far and then ignore it -- why not just drop it
> up-front?

You're probably right. I was thinking that we'd actually make use of it eventually in the browser reftests, but it's probably better to ignore it until then.

> That said, 'debug' also sounds like it refers to -DDEBUG, but I think you're
> talking about a run-time option rather than a compile-time option.  Is there a
> name that will make people more likely to expect that?

I guess that's what most of the other args are for -- choosing whether or not to run the test at all.

How about if I called it debugMode?

Or in case we wanted to add similar things,
  require(debugMode)
  runWith(debugMode)
  enable(debugMode)
  environment(debugMode)
  setup(debugMode)

Hmmm... setup(debugMode,pref:javascript.options.methodjit_always=true) could be fun.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

bug 635424 was nominated for blocking by jorendorff, so nominating this one for blocking

Not sure if it _should_, mind you!

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 513040 [details] [diff] [review]
Mark tests that need to run in debug mode

I don't know about blocking -- I'd personally release without it, without a second thought. However, I *do* think it should get approval, because it's a test-only change that fixes up a batch of useful tests. It was also recently broken (by me), though in truth the semantics haven't been quite what a reader would expect ever since async debugging came in many months ago. It just didn't matter with the specific tests involved (or they may have been modified to conceal the difference, I'm not sure.)

That said, this patch will probably mutate slightly before it makes it in, to be less of a special case (or at least use a less deceptive keyword.)

Comment 23

[Jason Orendorff [:jorendorff]]

I only nominated because I didn't know what the problem was, just that a test was exploding. I agree with sfink.

Comment 24

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

Agree, not blocking...

Comment 25

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

Comment on attachment 513040 [details] [diff] [review]
Mark tests that need to run in debug mode

...but Imma let you finish.

Comment 26

[Steve Fink [:sfink] [:s:]]

Attachment: Skip tests temporarily

For now, just skip the tests until we have time to decide on an appropriate manifest format.

Comment 27

[Jim Blandy :jimb]

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

Comment 28

[Robert Sayre]

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

Comment 29

[Steve Fink [:sfink] [:s:]]

I would like to leave this open until we get the proper fix in.

Comment 30

[Andreas Gal :gal]

Please file a new bug to track additional fixes.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 513040 [details] [diff] [review]
Mark tests that need to run in debug mode

Looks like this patch is obsoleted by attachment 516438 [details] [diff] [review] in bug 638274.