Closed Bug 414740 Opened 13 years ago Closed 13 years ago

nsLineIterator leak with caret mode and pressing right arrow key, using testcase from bug 344164

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: martijn.martijn, Assigned: mats)

References

()

Details

(Keywords: memory-leak, testcase)

Attachments

(3 files)

I'm getting this leak when pressing the right arrow key a couple of times with the testcase from bug 344164 and in caret mode:

== BloatView: ALL (cumulative) LEAK STATISTICS

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          23     2856   550885      102 ( 1092.46 +/-  1366.90)  1254928      102 ( 1465.73 +/-  2288.88)
 450 nsLineIterator                                 28     2856      488      102 (   48.40 +/-    34.25)      860      102 (   49.36 +/-    34.23)

There is also an automated testcase: https://bugzilla.mozilla.org/attachment.cgi?id=228724
You need to download it to your computer, because of the use of enhanced privileges.
It also seems to happen with https://bugzilla.mozilla.org/attachment.cgi?id=230293 , with the testcase from bug 345616.
Looks bad
Flags: blocking1.9+
Priority: -- → P2
Attached patch Patch rev. 1Splinter Review
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.927&root=/cvsroot&mark=328,333#317
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
Attachment #300337 - Flags: superreview?(roc)
Attachment #300337 - Flags: review?(roc)
Attached patch Branch patchSplinter Review
Please also add the testcase as a crash-test so that we'll test this for leaks once we have proper leak testing for automated tests.
Attached patch crashtest.diffSplinter Review
Comment on attachment 300337 [details] [diff] [review]
Patch rev. 1

yikes
Attachment #300337 - Flags: superreview?(roc)
Attachment #300337 - Flags: superreview+
Attachment #300337 - Flags: review?(roc)
Attachment #300337 - Flags: review+
Attachment #300337 - Flags: approval1.9?
Attachment #300337 - Flags: approval1.9?
Man.  Having QI on a frame actually return a refcounted object is evil...

Benjamin, how are we handling this case in your moz2 changes?
In bug 396185 nsILineIterator is still a refcounted object. But there at least you have pseudo-documentation for which objects are refcounted and which aren't: refcounted objects inherit from nsISupports while non-refcounted frames inherit from nsFrameQI or nothing.
Mats, does this cause the failure of 51 other crash-tests (i noticed you just commented out yours)?
mozilla/layout/generic/nsFrameList.cpp 	3.54
mozilla/layout/generic/crashtests/414740.html 	1.1

The testcase fails for unknown reasons though so I removed it from
the crashtests.list for now in an attempt to make Tinderbox green.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
(In reply to comment #10)
> Mats, does this cause the failure of 51 other crash-tests (i noticed you just
> commented out yours)?

Yes, I'm pretty sure this test caused the tests after it to fail,
maybe by causing a permission dialog to pop up?  I have removed it
so Tinderbox should go green now...
verified fixed with the steps to reproduce from mats and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre -> no leak ...verified fixed
Status: RESOLVED → VERIFIED
This bug's crashtest uses enablePrivilege, which isn't allowed for crashtests (pops up a permission dialog, as suggested in comment 12).

It should probably be converted into a mochitest, because the mochitest harness auto-allows enablePrivilege requests.
Depends on: 527804
Or we could fix bug 448676...
Depends on: 448676
You need to log in before you can comment on or make changes to this bug.