Changeset 19843 in webkit

Timestamp:

Feb 24, 2007 6:14:54 PM (17 years ago)

Author:

ggaren

Message:

LayoutTests:

Reviewed by Darin Adler.

Layout tests for BidiRun leaks.

• fast/leaks/001-expected.txt: Added.
• fast/leaks/001.html: Added.
• fast/leaks/002-expected.txt: Added.
• fast/leaks/002.html: Added.

WebCore:

Reviewed by Darin Adler.

Fixed <rdar://problem/4987649> leaks in BidiRun::operator new seen while
running WebKit unit tests

In bidi.cpp, some functions allocate BidiRuns and put them in a global data
structure, while others uses the BidiRuns in the global data structure.
The caller is responsible for knowing which functions may allocate runs
and which may use them, and calling deleteBidiRuns() at the appropriate time.

The fix is to add some calls to deleteBidiRuns() where they were missing.

I also added a BidiRun counter because these two leaks were introduced by
our two bidi.cpp experts, so the odds that leaks will creep in again
in the future seem pretty high.

• rendering/bidi.cpp: (WebCore::RenderBlock::bidiReorderCharacters): Added missing call to deleteBidiRuns(). (WebCore::BidiRunCounter::~BidiRunCounter): (WebCore::BidiRun::operator delete): (WebCore::RenderBlock::layoutInlineChildren): Added missing call to deleteBidiRuns(). Moved call to deleteBidiRuns() to same scope as call to bidiReorderLine(), to emphasize that they go together like new/delete. In theory, the old code was just as good, but I didn't want to rely on theory.

WebKitTools:

Reviewed by Darin Adler.

• Scripts/run-webkit-tests: Stop ignoring BidiRun leaks, now that they're fixed.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/leaks (added)
• LayoutTests/fast/leaks/001-expected.txt (added)
• LayoutTests/fast/leaks/001.html (added)
• LayoutTests/fast/leaks/002-expected.txt (added)
• LayoutTests/fast/leaks/002.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/rendering/bidi.cpp (modified) (8 diffs)
• WebKitTools/ChangeLog (modified) (1 diff)
• WebKitTools/Scripts/run-webkit-tests (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2007-02-24  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Darin Adler.
+        
+        Layout tests for BidiRun leaks.
+        
+        * fast/leaks/001-expected.txt: Added.
+        * fast/leaks/001.html: Added.
+        * fast/leaks/002-expected.txt: Added.
+        * fast/leaks/002.html: Added.
+
 2007-02-24  Alexey Proskuryakov  <ap@webkit.org>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2007-02-24  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Darin Adler.
+        
+        Fixed <rdar://problem/4987649> leaks in BidiRun::operator new seen while 
+        running WebKit unit tests
+        
+        In bidi.cpp, some functions allocate BidiRuns and put them in a global data
+        structure, while others uses the BidiRuns in the global data structure. 
+        The caller is responsible for knowing which functions may allocate runs
+        and which may use them, and calling deleteBidiRuns() at the appropriate time.
+
+        The fix is to add some calls to deleteBidiRuns() where they were missing.
+        
+        I also added a BidiRun counter because these two leaks were introduced by
+        our two bidi.cpp experts, so the odds that leaks will creep in again
+        in the future seem pretty high.
+
+        * rendering/bidi.cpp:
+        (WebCore::RenderBlock::bidiReorderCharacters): Added  missing call to
+        deleteBidiRuns().
+        (WebCore::BidiRunCounter::~BidiRunCounter):
+        (WebCore::BidiRun::operator delete):
+        (WebCore::RenderBlock::layoutInlineChildren): Added missing call to
+        deleteBidiRuns(). Moved call to deleteBidiRuns() to same scope as call to
+        bidiReorderLine(), to emphasize that they go together like new/delete.
+        In theory, the old code was just as good, but I didn't want to rely on
+        theory.
+
 2007-02-24  David Harrison  <harrison@apple.com>
 

trunk/WebCore/rendering/bidi.cpp

@@ -30 +30 @@
 #include "FrameView.h"
 #include "InlineTextBox.h"
+#include "Logging.h"
 #include "RenderArena.h"
 #include "RenderLayer.h"
@@ -112 +113 @@
 static void embed(Direction, BidiState&);
 static void appendRun(BidiState&);
+static void deleteBidiRuns(RenderArena*);
 
 void RenderBlock::bidiReorderCharacters(Document* document, RenderStyle* style, CharacterBuffer& characterBuffer)
@@ -169 +171 @@
     }
 
-    // Tear down temporary RenderBlock and RenderText
+    // Tear down temporary RenderBlock, RenderText, and BidiRuns
     block->removeChild(text);
     text->destroy();
     block->destroy();
+    deleteBidiRuns(document->renderArena());
 }
 
@@ -213 +216 @@
 
 #ifndef NDEBUG
+WTFLogChannel LogWebCoreBidiRunLeaks =  { 0x00000000, "", WTFLogChannelOn };
+
+struct BidiRunCounter { 
+    static int count; 
+    ~BidiRunCounter() 
+    { 
+        if (count)
+            LOG(WebCoreBidiRunLeaks, "LEAK: %d BidiRun\n", count);
+    }
+};
+int BidiRunCounter::count = 0;
+static BidiRunCounter bidiRunCounter;
+
 static bool inBidiRunDestroy;
 #endif
@@ -232 +248 @@
 void* BidiRun::operator new(size_t sz, RenderArena* renderArena) throw()
 {
+#ifndef NDEBUG
+    ++BidiRunCounter::count;
+#endif
     return renderArena->allocate(sz);
 }
@@ -237 +256 @@
 void BidiRun::operator delete(void* ptr, size_t sz)
 {
+#ifndef NDEBUG
+    --BidiRunCounter::count;
+#endif
     assert(inBidiRunDestroy);
 
@@ -1690 +1712 @@
             }
             end = findNextLineBreak(start, bidi);
-            if (start.atEnd())
+            if (start.atEnd()) {
+                deleteBidiRuns(renderArena());
                 break;
+            }
             if (!isLineEmpty) {
                 bidiReorderLine(start, end, bidi);
@@ -1723 +1747 @@
                             lineBox->addHighlightOverflow();
 #endif
-
-                        deleteBidiRuns(renderArena());
                     }
                 }
+
+                deleteBidiRuns(renderArena());
                 
                 if (end == start) {

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2007-02-24  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Darin Adler.
+        
+        * Scripts/run-webkit-tests: Stop ignoring BidiRun leaks, now that they're
+        fixed.
+
 2007-02-24  Krzysztof Kowalczyk  <kkowalczyk@gmail.com>
 

trunk/WebKitTools/Scripts/run-webkit-tests

@@ -853 +853 @@
         "NSURLCache cachedResponseForRequest", # leak in CFURL cache, Radar 4768430
         "CGImageSourceGetPropertiesAtIndex", # leak apparently in CG, Radar 4628809
-        "BidiRun::operator new", # leak in WebCore, Radar 4987649
         "gldGetString" # leak apparently in OpenGL, Radar 5013699
     );