All users were logged out of Bugzilla on October 13th, 2018

The check-interactive code executes head, tail scripts before the user runs

RESOLVED FIXED in mozilla1.9.2a1

Status

--
major
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: WeirdAl, Assigned: sgautherie)

Tracking

(Blocks: 1 bug, {autotest-issue, fixed1.9.1})

Trunk
mozilla1.9.2a1
autotest-issue, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1rc2: Dv1c; fixed1.9.1b4: Cv1], URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
Spin off of bug 382752.  In that bug, I realized there's a subtle flaw where the tail_extensionmanager.js code executes before the user can run the test.

From the other bug:
"One solution to this would be to wrap this code in a function that run_test or
one of its subsidiaries calls.  A better one would be to define in head.js a
couple arrays to store functions that _execute_test() must call on."

Right now, there's about a dozen head_*.js files that this bug would affect, and three tail_*.js files.  I can also write a testcase which demonstrates this problem.

Opinions on which solution to take?
(Reporter)

Comment 1

12 years ago
Created attachment 268276 [details] [diff] [review]
infrastructure patch

Fixes to individual head, tail files I'd like to write up as we go.  This patch simply adds the ability to handle the changes in head.js, tail.js.
Attachment #268276 - Flags: review?
(Reporter)

Updated

12 years ago
Attachment #268276 - Flags: review? → review?(rcampbell)
FWIW if I'm reading your suggestions right and you plan to change every head and tail js test file then I don't think it's the right way to go about this.
(Reporter)

Comment 3

12 years ago
I'm open to suggestions.

Comment 4

12 years ago
Comment on attachment 268276 [details] [diff] [review]
infrastructure patch

This flat-out isn't happening; I'll save robcee the trouble of looking at it.
Attachment #268276 - Flags: review?(rcampbell)
(Reporter)

Comment 5

12 years ago
Are you saying this bug is a WONTFIX?

Comment 6

12 years ago
No.  I'm saying the suggested solution to it is the wrong one, and I *very* much doubt this particular problem can't be solved without modifying all the head/tail-using tests.
(Reporter)

Comment 7

11 years ago
Created attachment 270842 [details] [diff] [review]
patch, v2

I realized a much better way to fix the bug.  Here, the scripts are loaded via the subscript loader (instead of at the command-line), when the user calls _execute_test().  You can only run _execute_test() once per xpcshell instance, but given the intent of check-interactive (to allow attaching a debugger), that's fine imho.

I have changed the sequencing slightly so that test-specific tail scripts load before the tail.js script.
Attachment #268276 - Attachment is obsolete: true
Attachment #270842 - Flags: review?(rcampbell)
(Reporter)

Comment 8

11 years ago
rob: any plans to review this patch?  It's been in your queue for over six months now.
whups. I kind of dropped this earlier and meant to get back to it.

Alex: Do you know if this patch is still relevant since the changes to check-interactive were landed?

see bug 416388.

If so, I'll get back to taking a look at this.
(In reply to comment #9)
> Alex: Do you know if this patch is still relevant since the changes to
> check-interactive were landed?

I can vouch for this still being a problem on today's cvs trunk. We could really do with a fix for this because its going to make debugging mailnews tests really hard (we need to clean up after the majority of tests).
Comment on attachment 270842 [details] [diff] [review]
patch, v2

passing review request over to waldo.
Attachment #270842 - Flags: review?(rcampbell) → review?(jwalden+bmo)

Comment 12

11 years ago
Comment on attachment 270842 [details] [diff] [review]
patch, v2

I think there's a load() builtin shell function which you can/should use instead of the subscript loader.  Also, please wrap each load in a try/catch and ensure test failure if a throw happens; I don't recall whether a throw would properly terminate it here or not.
Attachment #270842 - Flags: review?(jwalden+bmo) → review-

Comment 13

10 years ago
Alex, any plans to update this patch soon? 

We're gonna have to work around the bug in all tests in mailnews (which, luckily aren't *that* many - yet).
(Reporter)

Comment 14

10 years ago
hwaara - I haven't been nearly as active this past year in Moz development as I had hoped.  Even though it probably wouldn't take me that long to do it, I would probably need some nagging by e-mail to do it.  Just too many things on my plate - in and out of Mozilla.  Your best bet is on a weekend.  :)

Don't bother nagging on the bug itself - just send me a direct e-mail.

Updated

10 years ago
Blocks: 442026
(Reporter)

Comment 15

10 years ago
Also, what branches do you want this on?  That may trivially affect my work (1.9.0.x branch - CVS, 1.9.1 - hg).
(In reply to comment #15)
> Also, what branches do you want this on?  That may trivially affect my work
> (1.9.0.x branch - CVS, 1.9.1 - hg).
> 
definitely hg. Possibly cvs pending the current discussions.
(Reporter)

Comment 17

10 years ago
Waldo: a quick test shows that if the script file doesn't exist, load() doesn't throw... is that a problem?
It's definitely not good, though probably not considered show-stopping. If you can tighten up that code, feel free to drop a patch.

Also, Waldo's going to be hard to reach for the next month or so.
(Reporter)

Comment 19

10 years ago
robcee: I'll probably post a patch after bug 443220 gets fixed.  Right now, any patch I'd post would conflict with that bug and would need redoing.
(In reply to comment #18)
> Also, Waldo's going to be hard to reach for the next month or so.

Next 5 months or so, AFAIK.
 

Awhile yeah. Last update he still had a couple thousand miles to go(!).

(In reply to comment #19)
> robcee: I'll probably post a patch after bug 443220 gets fixed.  Right now, any
> patch I'd post would conflict with that bug and would need redoing.

Understood. Thanks for staying with this stuff!
(In reply to comment #17)
> Waldo: a quick test shows that if the script file doesn't exist, load() doesn't
> throw... is that a problem?

hwaara was poking at the xpcshell load() code recently, iirc.

Comment 23

10 years ago
(In reply to comment #17)
> Waldo: a quick test shows that if the script file doesn't exist, load() doesn't
> throw... is that a problem?

That's bug 439110 which was checked in, but then backed out due to test failures. I think that bug is not a showstopper for this bug though. Am I wrong?
(Reporter)

Comment 24

10 years ago
Whoops, I dropped the ball on this.  (There was a vacation in between.)  I may need weekly reminders.
(Reporter)

Comment 25

10 years ago
Requesting wanted1.9.1 for the mailnews testing requirement, and also in case someone wants to pick up this bug before I post another patch.
Flags: wanted1.9.1?
Whiteboard: [tb3needs]
(Assignee)

Updated

10 years ago
Version: unspecified → Trunk
(Assignee)

Comment 26

10 years ago
Alex, ping ?
Standard8: it doesn't look to me like this is specifically tied to shipping Thunderbird 3.  Is it?  If not, I'd suggest removing the [tb3needs] flag...
Whiteboard: [tb3needs] → [tb3needs][needs input Standard8]
(In reply to comment #27)
> Standard8: it doesn't look to me like this is specifically tied to shipping
> Thunderbird 3.  Is it?  If not, I'd suggest removing the [tb3needs] flag...

True, it doesn't block us for the release. It does however mean we can't implement tidy up on shutdown (without upsetting developers) which in turn means we can't use unit tests as an additional tool to check for potential memory leaks.
Severity: normal → major
Whiteboard: [tb3needs][needs input Standard8]
(Assignee)

Updated

10 years ago
Depends on: 439110
(Assignee)

Updated

10 years ago
Blocks: 469523
Status: NEW → ASSIGNED
Keywords: autotest-issue
Whiteboard: [patch needs unbitrotting]
(Assignee)

Comment 29

10 years ago
Note 'Bug 482084 - rewrite xpcshell test harness' landed.
(Reporter)

Comment 30

10 years ago
(In reply to comment #29)
> Note 'Bug 482084 - rewrite xpcshell test harness' landed.

I don't know Python.  Is there someone here who can take this bug over?
Assignee: ajvincent → nobody
Could we solve this simply by not loading the tail files at all in interactive mode? I mean, if you're debugging there's a decent chance you might not let it finish anyway. The tail scripts are likely to just be cleanup anyway.
(In reply to comment #31)
> Could we solve this simply by not loading the tail files at all in interactive
> mode? I mean, if you're debugging there's a decent chance you might not let it
> finish anyway. The tail scripts are likely to just be cleanup anyway.

True, but is there a chance folks will want them for attaching the debugger and looking at leaks on exit? I'm thinking this may be true in mailnews where we need to do clean up to avoid some leaks on exit (which we'll need for TUnit leak checking).
Ok, my other thought then is that we could make interactive mode not load the scripts with -f, but instead pass something like:
-e "function _load_tail_files() { load('/full/path/to/tail_bar.js'); load('/full/path/to/tail_foo.js'); }", and the info message for interactive mode say something like:
"Type _execute_test(); to run the test. When finished, type _load_tail_files(); to run cleanup code."
(Assignee)

Comment 34

10 years ago
(In reply to comment #30)
> Is there someone here who can take this bug over?

I'll do.
Assignee: nobody → sgautherie.bz
(Assignee)

Comment 35

10 years ago
Created attachment 367520 [details] [diff] [review]
(Cv1) Move _execute_test() definition to head.js from tail.js
[Checkin: Comment 36 & 37]

Bug 382682 changed this inline code into a function,
so there is no need for it to live in tail.js anymore.
Attachment #367520 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

10 years ago
Component: Build Config → TUnit
Flags: wanted1.9.1?
Product: Core → Testing
QA Contact: build-config → tunit
Whiteboard: [patch needs unbitrotting]
Target Milestone: --- → mozilla1.9.2a1

Updated

10 years ago
Attachment #367520 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 36

10 years ago
Comment on attachment 367520 [details] [diff] [review]
(Cv1) Move _execute_test() definition to head.js from tail.js
[Checkin: Comment 36 & 37]


http://hg.mozilla.org/mozilla-central/rev/1752f796d0aa
Attachment #367520 - Attachment description: (Cv1) Move _execute_test() definition to head.js from tail.js → (Cv1) Move _execute_test() definition to head.js from tail.js [Checkin: Comment 36]
(Assignee)

Comment 37

10 years ago
Comment on attachment 367520 [details] [diff] [review]
(Cv1) Move _execute_test() definition to head.js from tail.js
[Checkin: Comment 36 & 37]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/eaf873bfd86b
Attachment #367520 - Attachment description: (Cv1) Move _execute_test() definition to head.js from tail.js [Checkin: Comment 36] → (Cv1) Move _execute_test() definition to head.js from tail.js [Checkin: Comment 36 & 37]
(Assignee)

Updated

10 years ago
Whiteboard: [fixed1.9.1b4: Cv1]
(Assignee)

Comment 38

10 years ago
Created attachment 371184 [details] [diff] [review]
(Dv1) Load tail files after test execution, as expected

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090402 Minefield/3.6a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/c4f6bb1db611)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090402 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/c4f6bb1db611
 +http://hg.mozilla.org/comm-central/rev/cdfcaddd6832)

I did not do it for the head files,
as I considered the main need is to not execute the tail files before the test,
not to debug heads and tails this way.
(This could easily be added if need be.)

This is "patch, v2" rewritten in python and more ;-)
Attachment #270842 - Attachment is obsolete: true
Attachment #371184 - Flags: review?(jwalden+bmo)
(In reply to comment #38)
> Created an attachment (id=371184) [details]
> (Dv1) Load tail files after test execution, as expected

This patch looks really good and will finally enable a good solution for getting mailnews xpcshell test shutting down cleanly and keeping developers happy...

Jeff, any chance of a review soon?
(Assignee)

Comment 40

10 years ago
Created attachment 374604 [details] [diff] [review]
(Dv1a) Load head, test and tail files from _execute_test()

Dv1, with your suggestions from your recent irc review.

"... load is variadic or not, but fileList.forEach(load) ..."
It seems it is not: I tried it and got
|testing\xpcshell\head.js:124: Error: cannot open file '0' for reading|

As the test head files moved, the test file had to too.
Attachment #371184 - Attachment is obsolete: true
Attachment #374604 - Flags: review?(jwalden+bmo)
Attachment #371184 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

10 years ago
Blocks: 490147
(Assignee)

Comment 41

10 years ago
Jeff, ping for review?
(Assignee)

Comment 42

10 years ago
Jeff, ping for review?
(Assignee)

Comment 43

10 years ago
Comment on attachment 374604 [details] [diff] [review]
(Dv1a) Load head, test and tail files from _execute_test()

Jeff reviewed the first patch over irc.
Rob, would you take over the new patch review?
Attachment #374604 - Flags: review?(rcampbell)
(In reply to comment #40)
> Created an attachment (id=374604) [details]
> (Dv1a) Load head, test and tail files from _execute_test()

This seems to have bitrotted, I can't apply to trunk or branch at the moment.
(Assignee)

Comment 45

10 years ago
Created attachment 381309 [details] [diff] [review]
(Dv1b) Load head, test and tail files from _execute_test()

Dv1a, unbitrotted.

Jeff, Rob, this is still very much wanted asap, as, for example, it blocks bug 469523 which prevents catching leak regressions on a timely manner :-/
Attachment #374604 - Attachment is obsolete: true
Attachment #381309 - Flags: review?(rcampbell)
Attachment #381309 - Flags: review?(jwalden+bmo)
Attachment #374604 - Flags: review?(rcampbell)
Attachment #374604 - Flags: review?(jwalden+bmo)
Jeff, do you have time to review this?
(Assignee)

Comment 47

10 years ago
Created attachment 381678 [details] [diff] [review]
(Dv1c) Load head, test and tail files from _execute_test()
[Checkin: Comment 48 & 49]

Dv1b, unbitrotted after bug 362433.
Attachment #381309 - Attachment is obsolete: true
Attachment #381678 - Flags: review?(rcampbell)
Attachment #381678 - Flags: review?(jwalden+bmo)
Attachment #381309 - Flags: review?(rcampbell)
Attachment #381309 - Flags: review?(jwalden+bmo)

Updated

10 years ago
Attachment #381678 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 48

10 years ago
Comment on attachment 381678 [details] [diff] [review]
(Dv1c) Load head, test and tail files from _execute_test()
[Checkin: Comment 48 & 49]


http://hg.mozilla.org/mozilla-central/rev/eaec174cb9c2
Attachment #381678 - Attachment description: (Dv1c) Load head, test and tail files from _execute_test() → (Dv1c) Load head, test and tail files from _execute_test() [Checkin: Comment 48]
Attachment #381678 - Flags: review?(rcampbell)
(Assignee)

Updated

10 years ago
Depends on: 362433
(Assignee)

Comment 49

10 years ago
Comment on attachment 381678 [details] [diff] [review]
(Dv1c) Load head, test and tail files from _execute_test()
[Checkin: Comment 48 & 49]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a08e2e4571d6
Attachment #381678 - Attachment description: (Dv1c) Load head, test and tail files from _execute_test() [Checkin: Comment 48] → (Dv1c) Load head, test and tail files from _execute_test() [Checkin: Comment 48 & 49]
(Assignee)

Comment 50

10 years ago
(In reply to comment #49)
> (From update of attachment 381678 [details] [diff] [review])
> 
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a08e2e4571d6

After fixing context for
{
patching file testing/xpcshell/head.js
Hunk #1 FAILED at 64
1 out of 3 hunks FAILED
patching file testing/xpcshell/runxpcshelltests.py
Hunk #1 FAILED at 71
1 out of 3 hunks FAILED
}
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
No longer depends on: 362433
Flags: in-testsuite-
Keywords: fixed1.9.1
Resolution: --- → FIXED
Whiteboard: [fixed1.9.1b4: Cv1] → [fixed1.9.1rc1+: Dv1c; fixed1.9.1b4: Cv1]
(Assignee)

Updated

10 years ago
Whiteboard: [fixed1.9.1rc1+: Dv1c; fixed1.9.1b4: Cv1] → [fixed1.9.1rc2: Dv1c; fixed1.9.1b4: Cv1]
You need to log in before you can comment on or make changes to this bug.