Closed
Bug 369034
Opened 17 years ago
Closed 14 years ago
TestTXMgr has error and leaks object
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
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)
7.39 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
13.20 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
31.77 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
10.54 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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 %
Updated•17 years ago
|
Component: General → Editor
QA Contact: general
Updated•17 years ago
|
QA Contact: editor
Assignee | ||
Comment 1•15 years ago
|
||
[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.
Assignee | ||
Comment 2•15 years ago
|
||
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)
Assignee | ||
Comment 3•15 years ago
|
||
The error is a regression from http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/editor/txmgr/tests/TestTXMgr.cpp&rev=MOZILLA_1_8_BRANCH&mark=1.48 The (current) leak would be from http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/editor/txmgr/tests/TestTXMgr.cpp&rev=MOZILLA_1_8_BRANCH&mark=1.43
Flags: in-testsuite+
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Comment 4•15 years ago
|
||
You'll want to use :bs (aka benjamin@smedbergs) rather than that bsmedberg@gmail watcher account for the review request.
Assignee | ||
Updated•15 years ago
|
Attachment #374594 -
Flags: review?(bsmedberg) → review?(benjamin)
Assignee | ||
Comment 5•15 years ago
|
||
Please, double check Makefile.in changes.
Attachment #374594 -
Attachment is obsolete: true
Attachment #374669 -
Flags: review?(benjamin)
Attachment #374594 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #374669 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 6•15 years ago
|
||
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]
Assignee | ||
Comment 7•15 years ago
|
||
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]
Assignee | ||
Updated•15 years ago
|
Keywords: fixed1.9.1
Whiteboard: [fixed1.9.1b5]
Assignee | ||
Comment 8•15 years ago
|
||
(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 9•15 years ago
|
||
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)
Assignee | ||
Comment 10•15 years ago
|
||
(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.)
Comment 11•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
Per comment 11.
Attachment #376331 -
Attachment is obsolete: true
Attachment #376796 -
Flags: review?(peterv)
Attachment #376331 -
Flags: review?(peterv)
Comment 13•15 years ago
|
||
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?
Assignee | ||
Comment 14•15 years ago
|
||
(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 15•15 years ago
|
||
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+
Assignee | ||
Comment 16•15 years ago
|
||
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]
Assignee | ||
Comment 17•15 years ago
|
||
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]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [fixed1.9.1b5] → [fixed1.9.1rc1: Av1a; fixed1.9.1...: Cv1]
Assignee | ||
Comment 18•15 years ago
|
||
(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)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [fixed1.9.1rc1: Av1a; fixed1.9.1...: Cv1] → [fixed1.9.1b99: Av1a; fixed1.9.1rc1: Cv1]
Updated•14 years ago
|
Attachment #382211 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 19•14 years ago
|
||
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]
Assignee | ||
Updated•14 years ago
|
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
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #433775 -
Flags: review?(peterv)
Assignee | ||
Comment 21•14 years ago
|
||
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]
Assignee | ||
Updated•14 years ago
|
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]
Assignee | ||
Updated•14 years ago
|
Attachment #433775 -
Flags: review?(ehsan)
Updated•14 years ago
|
Attachment #433775 -
Flags: review?(peterv)
Attachment #433775 -
Flags: review?(ehsan)
Attachment #433775 -
Flags: review+
Assignee | ||
Comment 22•14 years ago
|
||
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]
Comment 23•14 years ago
|
||
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.
Comment 24•14 years ago
|
||
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
Assignee | ||
Comment 26•14 years ago
|
||
(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
Comment 27•14 years ago
|
||
"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.
Comment 28•14 years ago
|
||
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?
Assignee | ||
Comment 29•14 years ago
|
||
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.
Description
•