Closed Bug 470418 Opened 11 years ago Closed 11 years ago

Leak nsStyleContext with RTL, text-transform

Categories

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

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: jruderman, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(5 keywords)

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Attached file leak report
hg bisect says this is a regression from bug 441782.
Blocks: 441782
Flags: blocking1.9.1?
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?
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?
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).
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.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
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.
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.
Depends on: 471389
This seems to fix the leak.  Johnathan, are you going to ask review on your patch?
OS: Mac OS X → All
Yes, I'll request review shortly, just wanted to let my tryserver build finish to check that nothing breaks there.
Attachment #354952 - Flags: superreview?(roc)
Attachment #354952 - Flags: review?(ehsan.akhgari)
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?
Attachment #354952 - Flags: review?(ehsan.akhgari) → review+
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.
Attachment #354952 - Attachment is obsolete: true
Attachment #354968 - Flags: superreview?(roc)
Attachment #354952 - Flags: superreview?(roc)
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.  :-)
Attachment #354968 - Flags: review?(roc)
Attachment #354968 - Flags: superreview?(roc)
Attachment #354968 - Flags: superreview+
Attachment #354968 - Flags: review?(roc)
Attachment #354968 - Flags: review+
Blocks: 471389
No longer depends on: 471389
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?
Landed on mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/00f1c4de50b8>

Thanks for your patch, Jonathan!
Assignee: nobody → jfkthame
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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.
Attachment #354968 - Flags: approval1.9.1?
(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?
Flags: in-testsuite+
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.
Attachment #354968 - Flags: approval1.9.1?
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.