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

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

It also seems to happen with https://bugzilla.mozilla.org/attachment.cgi?id=230293 , with the testcase from bug 345616.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Looks bad

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.927&root=/cvsroot&mark=328,333#317

Comment 4

[Mats Palmgren (inactive)]

Attachment: Branch patch

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Mats Palmgren (inactive)]

Attachment: crashtest.diff

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 300337 [details] [diff] [review]
Patch rev. 1

yikes

Comment 8

[Boris Zbarsky [:bzbarsky]]

Man.  Having QI on a frame actually return a refcounted object is evil...

Benjamin, how are we handling this case in your moz2 changes?

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Asaf Romano (gone)]

Mats, does this cause the failure of 51 other crash-tests (i noticed you just commented out yours)?

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Mats Palmgren (inactive)]

(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...

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Daniel Holbert [:dholbert]]

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.

Comment 15

[Mats Palmgren (inactive)]

Or we could fix bug 448676...