470418 - Leak nsStyleContext with RTL, text-transform

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase and quitting makes trace-refcnt report leaked nsStyleContext and gfxTextRun objects.  I think this bug appeared within the last few days on mozilla-central.

Comment 1

[Jesse Ruderman]

Attachment: leak report

Comment 2

[Boris Zbarsky [:bzbarsky]]

hg bisect says this is a regression from bug 441782.

Comment 3

[Boris Zbarsky [:bzbarsky]]

What's particularly odd is that I'm seeing a style context leaked because an nsTextFrame takes a reference to it and never releases it.  But we're not leaking any frames or content nodes...

Is this a matter of us just releasing some things too late during shutdown?

Comment 4

[(no longer active)]

I get the below assertion when loading the test case in a debug build:

ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file c:/mozroot/hgmoz/layout/base/nsPresShell.cpp, line 675

I'm no expert here, but it seems like some parts of a frame are not being freed.  Does this help in tracking this down?  Where should I move from here?

Comment 5

[Jesse Ruderman]

That assertion failure is just a side effect of leaking nsStyleContexts, since nsStyleContexts are allocated from the same arena as frames (see "nsStyleContext::operator new").  To debug this, you'll have to track down the nsTextFrame leak (but see comment 3).

Comment 6

[(no longer active)]

I tried generating the shutdown log using the instructions at <https://developer.mozilla.org/en/Debugging_memory_leaks#Analyzing_the_shutdown_log>, but the output of this log shows nothing relevant.  There is no occurrence of gfxTextRun in the log.

Comment 7

[Jonathan Kew [:jfkthame]]

I believe this is caused by a mistake in the patch for bug 441782:

>diff --git a/gfx/thebes/public/gfxTextRunWordCache.h b/gfx/thebes/public/gfxTextRunWordCache.h
>--- a/gfx/thebes/public/gfxTextRunWordCache.h
>+++ b/gfx/thebes/public/gfxTextRunWordCache.h
>@@ -46,7 +46,15 @@
>   */
> class THEBES_API gfxTextRunWordCache {
> public:
>-    enum { TEXT_IN_CACHE = 0x10000000 };
>+    enum {
>+      TEXT_IN_CACHE = 0x10000000,
>+      /**
>+       * When set, the previous character for this textrun was an Arabic
>+       * character.  This is used for the context detection necessary for
>+       * bidi.numeral implementation.
>+       */
>+      TEXT_INCOMING_ARABICCHAR = 0x10000000
>+    };
> 
>     /**
>      * Create a textrun using cached words.

This uses the same bit in the flags for both TEXT_IN_CACHE and TEXT_INCOMING_ARABICCHAR, which is invalid. We should reorganize the flag bits slightly, as we're running out of available "user" bits; I will attach a patch after some further testing here.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See bug 471389.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: resolve conflict in text-run flags causing leakage

This patch moves the TEXT_{INCOMING,TRAILING}_ARABICCHAR flags in order to avoid overlapping the TEXT_IN_CACHE flag. This appears to resolve the reported leakage.

Comment 10

[(no longer active)]

This seems to fix the leak.  Johnathan, are you going to ask review on your patch?

Comment 11

[Jonathan Kew [:jfkthame]]

Yes, I'll request review shortly, just wanted to let my tryserver build finish to check that nothing breaks there.

Comment 12

[(no longer active)]

Comment on attachment 354952 [details] [diff] [review]
resolve conflict in text-run flags causing leakage

>diff --git a/gfx/thebes/public/gfxTextRunWordCache.h b/gfx/thebes/public/gfxTextRunWordCache.h
>-      /**
>
>-       * When set, the previous character for this textrun was an Arabic
>
>-       * character.  This is used for the context detection necessary for
>
>-       * bidi.numeral implementation.
>
>-       */

Any special reason to remove this comment?

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: updated patch to restore comment (see comment #12)

There was no reason to delete that comment, it was inadvertently left out during various rearrangements of the code. I've updated the patch to restore it, as it's a helpful note about what's going on.

Comment 14

[(no longer active)]

Comment on attachment 354968 [details] [diff] [review]
updated patch to restore comment (see comment #12)

BTW, I don't think I'm eligible to give reviews officially, so you still need roc's review on this as well.  :-)

Comment 15

[Jonathan Kew [:jfkthame]]

AFAIK, this is ready to land, but I can't edit the Keywords to add "checkin-needed" (I guess that's because it isn't one of "my bugs"?)  Can someone either take care of this or assign it to me?

Comment 16

[(no longer active)]

Landed on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/00f1c4de50b8>

Thanks for your patch, Jonathan!

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354968 [details] [diff] [review]
updated patch to restore comment (see comment #12)

We'll want this on the 1.9.1 branch after it bakes.

Comment 18

[(no longer active)]

(In reply to comment #17)
> (From update of attachment 354968 [details] [diff] [review])
> We'll want this on the 1.9.1 branch after it bakes.

This is already a blocker; should it need explicit approval?

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354968 [details] [diff] [review]
updated patch to restore comment (see comment #12)

No, my fault not noticing that it was a blocker.

Comment 20

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a01eeb05c954

Comment 21

[Aakash Desai [:aakashd]]

verified FIXED using the attached test case (no assertion) on debug builds:


Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028