Closed Bug 369034 Opened 17 years ago Closed 14 years ago

TestTXMgr has error and leaks object

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: ray, Assigned: sgautherie)

References

()

Details

(Keywords: fixed1.9.1, memory-leak, regression, Whiteboard: [Av1a: fixed1.9.1b99; Cv1: fixed1.9.1rc1; Dv1: fixed1.9.3a3, fixed1.9.2.14, fixed1.9.1.17])

Attachments

(4 files, 2 obsolete files)

This should go into the component for the Composer module owned by Glazman. And the component for that module would be....

I found this by building Firefox with options:
mk_add_options MOZ_CO_PROJECT=browser
ac_add_options --enable-application=browser
ac_add_options --enable-accessibility=yes
ac_add_options --enable-debug=yes


% ./TestTXMgr
-----------------------------------------------------
- Begin Simple Transaction Test:
-----------------------------------------------------
Create transaction manager instance ... passed
Call DoTransaction() with null transaction ... passed
Call UndoTransaction() with empty undo stack ... passed
Call RedoTransaction() with empty redo stack ... passed
Call SetMaxTransactionCount(-1) with empty undo and redo stacks ... passed
Call SetMaxTransactionCount(0) with empty undo and redo stacks ... passed
Call SetMaxTransactionCount(10) with empty undo and redo stacks ... passed
Call Clear() with empty undo and redo stack ... passed
Call GetNumberOfUndoItems() with empty undo stack ... passed
Call GetNumberOfRedoItems() with empty redo stack ... passed
Call PeekUndoStack() with empty undo stack ... passed
Call PeekRedoStack() with empty undo stack ... passed
Call AddListener() with null listener ... passed
Call RemoveListener() with null listener ... passed
Test coalescing of transactions ... passed
Execute 20 transactions ... passed
Execute 20 transient transactions ... passed
Undo 4 transactions ... passed
Redo 2 transactions ... passed
Check if new transactions prune the redo stack ... passed
Undo 4 transactions then clear the undo and redo stacks ... passed
Execute 5 transactions ... passed
Test transaction DoTransaction() error ... passed
Test transaction UndoTransaction() error ... passed
Test transaction RedoTransaction() error ... passed
Test max transaction count of zero ... passed
Test SetMaxTransactionCount() greater than num stack items ... passed
Test SetMaxTransactionCount() pruning undo stack ... passed
Test SetMaxTransactionCount() pruning redo stack ... passed
Release the transaction manager ... passed
Number of transactions created and destroyed match ... ERROR: Transaction constructor count (131) != destructor count (111).
nsStringStats
 => mAllocCount:            191
 => mReallocCount:            0
 => mFreeCount:             190  --  LEAKED 1 !!!
 => mShareCount:              0
 => mAdoptCount:              1
 => mAdoptFreeCount:          1
%
Component: General → Editor
QA Contact: general
QA Contact: editor
Blocks: 489728
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090417 Minefield/3.6a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/b427fd627d36)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090417 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/9e06217fc471
 +http://hg.mozilla.org/comm-central/rev/...)

Error still there:
{
[...]
Number of transactions created and destroyed match ... ERROR: Transaction constr
uctor count (131) != destructor count (111).
}

First, get the test to fully pass;
then, see about (remaining) leak.
Blocks: mlkTests
Keywords: mlk
Before:
{
Number of transactions created and destroyed match ... ERROR: Transaction constr
uctor count (131) != destructor count (111).
nsStringStats
 => mAllocCount:           1275
 => mReallocCount:            3
 => mFreeCount:            1041  --  LEAKED 234 !!!
 => mShareCount:            945
 => mAdoptCount:              2
 => mAdoptFreeCount:          2
}

After:
{
2630250 transactions processed during stress test.

== BloatView: ALL (cumulative) LEAK STATISTICS
nsTraceRefcntImpl::DumpStatistics: 37 entries
nsStringStats
 => mAllocCount:           1275
 => mReallocCount:            3
 => mFreeCount:            1275
 => mShareCount:            945
 => mAdoptCount:              2
 => mAdoptFreeCount:          2
}
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #374594 - Flags: review?(bsmedberg)
You'll want to use :bs (aka benjamin@smedbergs) rather than that bsmedberg@gmail watcher account for the review request.
Attachment #374594 - Flags: review?(bsmedberg) → review?(benjamin)
Please, double check Makefile.in changes.
Attachment #374594 - Attachment is obsolete: true
Attachment #374669 - Flags: review?(benjamin)
Attachment #374594 - Flags: review?(benjamin)
Attachment #374669 - Flags: review?(benjamin) → review+
Comment on attachment 374669 [details] [diff] [review]
(Av1a) Add mgr->Clear(), use ScopedXPCOM() and run this test
[Checkin: Comment 6 & 7]


http://hg.mozilla.org/mozilla-central/rev/9785c406213b
Attachment #374669 - Attachment description: (Av1a) Add mgr->Clear(), use ScopedXPCOM() and run this test → (Av1a) Add mgr->Clear(), use ScopedXPCOM() and run this test [Checkin: Comment 6]
Comment on attachment 374669 [details] [diff] [review]
(Av1a) Add mgr->Clear(), use ScopedXPCOM() and run this test
[Checkin: Comment 6 & 7]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2e5765d922e4
Attachment #374669 - Attachment description: (Av1a) Add mgr->Clear(), use ScopedXPCOM() and run this test [Checkin: Comment 6] → (Av1a) Add mgr->Clear(), use ScopedXPCOM() and run this test [Checkin: Comment 6 & 7]
Keywords: fixed1.9.1
Whiteboard: [fixed1.9.1b5]
(This patch is much larger than it should be, due to 'hg diff' bug: try and (re)view it with another tool...)
Attachment #376331 - Flags: review?(benjamin)
Comment on attachment 376331 [details] [diff] [review]
(Bv1) Various code fixes and updates

I can't tell what part of this patch is leak fixes and what part is "random cleanup". I don't think it's worth committing random cleanup to this test, but I'm not a peer of this code so I'll let peterv decide.
Attachment #376331 - Flags: review?(benjamin) → review?(peterv)
(In reply to comment #9)
> (From update of attachment 376331 [details] [diff] [review])
> I can't tell what part of this patch is leak fixes and what part is "random

Patch A fixed error and leak.

> cleanup". I don't think it's worth committing random cleanup to this test, but

This is code cleanup I noticed when I read the code to do the first patch.

> I'm not a peer of this code so I'll let peterv decide.

(Fwiw, this is to get the code right, before looking into bug 489728.)
What bsmedberg said. I can review a patch with functional changes but I'm not going to bother reviewing it with all these whitespace changes and random cleanup.
Per comment 11.
Attachment #376331 - Attachment is obsolete: true
Attachment #376796 - Flags: review?(peterv)
Attachment #376331 - Flags: review?(peterv)
Comment on attachment 376796 [details] [diff] [review]
(Cv1) Functional changes part
[Checkin: Comment 16 & 17]

Why do you treat NS_SUCCEEDED(result) as an error?
(In reply to comment #13)
> Why do you treat NS_SUCCEEDED(result) as an error?

Because these very cases are designed to fail, so I think we should get only the very expected error.
Comment on attachment 376796 [details] [diff] [review]
(Cv1) Functional changes part
[Checkin: Comment 16 & 17]

>diff --git a/editor/txmgr/tests/TestTXMgr.cpp b/editor/txmgr/tests/TestTXMgr.cpp

>@@ -2505,17 +2509,17 @@ quick_test(TestTransactionFactory *facto

>-  if (u1 == u2 || u2) {
>+  if (u2) {

Why?
Attachment #376796 - Flags: review?(peterv) → review+
Comment on attachment 376796 [details] [diff] [review]
(Cv1) Functional changes part
[Checkin: Comment 16 & 17]


http://hg.mozilla.org/mozilla-central/rev/33089e675074


(In reply to comment #15)

> >-  if (u1 == u2 || u2) {
> >+  if (u2) {
> 
> Why?

To match
>     printf("ERROR: Unexpected item at top of undo stack. (%d)\n", result);

|u1 == u2| would catch |0 == 0| only, which can not be (in this part of the test, per previous checks).
Attachment #376796 - Attachment description: (Cv1) Functional changes part → (Cv1) Functional changes part [Checkin: Comment 16]
Comment on attachment 376796 [details] [diff] [review]
(Cv1) Functional changes part
[Checkin: Comment 16 & 17]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db8c7dd92ece
Attachment #376796 - Attachment description: (Cv1) Functional changes part [Checkin: Comment 16] → (Cv1) Functional changes part [Checkin: Comment 16 & 17]
Whiteboard: [fixed1.9.1b5] → [fixed1.9.1rc1: Av1a; fixed1.9.1...: Cv1]
(This patch is much larger than it should be, due to 'hg diff' bug: try and
(re)view it with another tool...)
Attachment #382211 - Flags: review?(peterv)
Whiteboard: [fixed1.9.1rc1: Av1a; fixed1.9.1...: Cv1] → [fixed1.9.1b99: Av1a; fixed1.9.1rc1: Cv1]
Attachment #382211 - Flags: review?(peterv) → review+
Comment on attachment 382211 [details] [diff] [review]
(Dv1) Fix and reorder printf()s
[Checkin: Comment 19 & 21]


http://hg.mozilla.org/mozilla-central/rev/b9424d98afb4
Attachment #382211 - Attachment description: (Dv1) Fix and reorder printf()s → (Dv1) Fix and reorder printf()s [Checkin: Comment 19]
Whiteboard: [fixed1.9.1b99: Av1a; fixed1.9.1rc1: Cv1] → [Av1a: fixed1.9.1b99; Cv1: fixed1.9.1rc1; Dv1: fixed1.9.3a3]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.3a3
Comment on attachment 382211 [details] [diff] [review]
(Dv1) Fix and reorder printf()s
[Checkin: Comment 19 & 21]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d4fc331d19f8

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d58681d8dd4d
Attachment #382211 - Attachment description: (Dv1) Fix and reorder printf()s [Checkin: Comment 19] → (Dv1) Fix and reorder printf()s [Checkin: Comment 19 & 21]
Whiteboard: [Av1a: fixed1.9.1b99; Cv1: fixed1.9.1rc1; Dv1: fixed1.9.3a3] → [Av1a: fixed1.9.1b99; Cv1: fixed1.9.1rc1; Dv1: fixed1.9.3a3, fixed1.9.2.14, fixed1.9.1.17]
Attachment #433775 - Flags: review?(ehsan)
Attachment #433775 - Flags: review?(peterv)
Attachment #433775 - Flags: review?(ehsan)
Attachment #433775 - Flags: review+
Comment on attachment 433775 [details] [diff] [review]
(Ev1) Whitespace cleanup, Var declaration reordering++
[Checked in: Comment 22 & 29]

http://hg.mozilla.org/mozilla-central/rev/53914a97b478
Attachment #433775 - Attachment description: (Ev1) Whitespace cleanup, Var declaration reordering++. → (Ev1) Whitespace cleanup, Var declaration reordering++ [Checked in: Comment 22]
The comment 22 changeset was landed as DONTBUILD.  This is NOT a change that can skip building; it substantively changes code.  Please do not ever do that again.
And what happened to one change per bug?
The issues described in comment 0 appear to no longer be present.

Since they don't appear to have been fixed by the other patches in this bug (if they were, the bug would have been marked FIXED by the person landing them), I'm resolving this bug as WORKSFORME.  Please don't use this bug for landing further changes.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
(In reply to comment #23)

> it substantively changes code.

It is test only.

> Please do not ever do that again.

As you wish.


(In reply to comment #24)
> And what happened to one change per bug?

I don't know. Can you remind me where that policy is explained?


(In reply to comment #25)

> The issues described in comment 0 appear to no longer be present.

Comment 0 is outdated.

> Since they don't appear to have been fixed by the other patches in this bug (if
> they were, the bug would have been marked FIXED by the person landing them),

Comment 10 already answer that...

> I'm resolving this bug as WORKSFORME.  Please don't use this bug for landing
> further changes.

So much nonsense for so trivial mess fixes:
First, I did patch B which was refused as modifying too many things at once.
Then I split it into smaller patches, and now I get complains that I'm landing multiple patches :-/
And all that for test code almost noone but me seems to care about...
Resolution: WORKSFORME → FIXED
"test only" is not a valid use case for DONTBUILD. This is at least the second time you've abused it. Please avoid using DONTBUILD again until you are certain that you understand its purpose.
Serge, I've also recently asked you to close bugs after checking in fixes for the stated problem. This bug has remained open for nearly a year and this is your fourth checkin with no clear indication that you're even working towards fixing the problem. Please keep your work focused to the bug in question.

> > The issues described in comment 0 appear to no longer be present.
> 
> Comment 0 is outdated.
>
> > Since they don't appear to have been fixed by the other patches in this bug (if
> > they were, the bug would have been marked FIXED by the person landing them),
>
> Comment 10 already answer that...

Then why wasn't the bug closed then?
Comment on attachment 433775 [details] [diff] [review]
(Ev1) Whitespace cleanup, Var declaration reordering++
[Checked in: Comment 22 & 29]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7e5b7647ca46

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f25deaf5e312
Attachment #433775 - Attachment description: (Ev1) Whitespace cleanup, Var declaration reordering++ [Checked in: Comment 22] → (Ev1) Whitespace cleanup, Var declaration reordering++ [Checked in: Comment 22 & 29]
You need to log in before you can comment on or make changes to this bug.