Closed Bug 634648 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: cdleary, Assigned: sfink)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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.
Assignee: general → sphink
(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.
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
(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.
Sounds like time to get some 8-core boxes for tbox.
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.
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.
Attachment #512873 - Flags: review?(dmandelin)
(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 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.
(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.
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...
  }
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.)
(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.)
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.
Attachment #513040 - Flags: review?(dmandelin)
Attachment #512873 - Attachment is obsolete: true
Attachment #512873 - Flags: review?(dmandelin)
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 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.
Attachment #513040 - Flags: review?(dmandelin)
Attachment #513040 - Flags: review?(bclary)
Attachment #513040 - Flags: review+
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.
Attachment #513040 - Flags: review?(bclary) → review?(dbaron)
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?
(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.
Duplicate of this bug: 635424
bug 635424 was nominated for blocking by jorendorff, so nominating this one for blocking

Not sure if it _should_, mind you!
blocking2.0: --- → ?
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.)
Attachment #513040 - Flags: approval2.0?
I only nominated because I didn't know what the problem was, just that a test was exploding. I agree with sfink.
Agree, not blocking...
blocking2.0: ? → -
Comment on attachment 513040 [details] [diff] [review]
Mark tests that need to run in debug mode

...but Imma let you finish.
Attachment #513040 - Flags: approval2.0? → approval2.0+
For now, just skip the tests until we have time to decide on an appropriate manifest format.
Attachment #514683 - Flags: review?(dmandelin)
Attachment #514683 - Flags: approval2.0?
Attachment #514683 - Flags: review?(dmandelin)
Attachment #514683 - Flags: review+
Attachment #514683 - Flags: approval2.0?
Attachment #514683 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/c8128fa8a368
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I would like to leave this open until we get the proper fix in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: TM: debug shell reftest assertion: !CompartmentHasLiveScripts(comp) → re-enable tests disabled due to debug shell reftest assertion: !CompartmentHasLiveScripts(comp)
Please file a new bug to track additional fixes.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Blocks: 638274
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.
Attachment #513040 - Attachment is obsolete: true
Attachment #513040 - Flags: review?(dbaron)
Blocks: 632343
You need to log in before you can comment on or make changes to this bug.