Bug 33731

Summary: [Dupe of] Many false leaks in release builds due to PtrAndFlags
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: JavaScriptCoreAssignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dglazkov, eric, joepeck, levin, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
The patch
oliver: review+
The patch
none
The patch
barraclough: review-
New implementation for UStringImpl.
none
New WebCore::StringImpl patch
none
WebCore::StringImpl patch take 3, re-introduce String::charactersWithNullTermination() / StringImpl::hasTerminatingNullCharacter()
barraclough: review-
take 4. Simple approach, add a flag back in for HasTerminatingNullCharacter.
barraclough: review-
New UStringImpl patch, without ancillary refactoring changes.
barraclough: review-
New StringImpl patch, also fix bufferIsInternal bug that was being triggered.
sam: review+
Last patch v0.1! Needs the ChnageLog filling out.
none
Final Patch! Refactor StructureTransitionTable back into Structure, remove PtrAndFlags! darin: review+

Description Gavin Barraclough 2010-01-15 12:40:48 PST
These break the Leaks tool on OS X.
Comment 1 Gavin Barraclough 2010-01-15 12:47:01 PST
Created attachment 46700 [details]
The patch
Comment 2 Gavin Barraclough 2010-01-15 12:49:57 PST
Created attachment 46702 [details]
The patch
Comment 3 Gavin Barraclough 2010-01-15 12:54:10 PST
Created attachment 46704 [details]
The patch
Comment 4 Oliver Hunt 2010-01-15 12:58:58 PST
Comment on attachment 46700 [details]
The patch

r-, i think 
cachedPrototypeStructure = (Structure*)1
 should be

static const Structure* seenFlag = (Structure*)1;

cachedPrototypeStructure = seenFlag

(with a better name for seenFlag)
Comment 5 Oliver Hunt 2010-01-15 13:01:34 PST
Comment on attachment 46704 [details]
The patch

i'd like maciej's opinion on stripping a bit from the string hash
Comment 6 Oliver Hunt 2010-01-15 13:02:24 PST
Comment on attachment 46700 [details]
The patch

bah, use a #define, otherwise r=me :(
Comment 7 Oliver Hunt 2010-01-15 13:08:18 PST
Comment on attachment 46704 [details]
The patch

r=me assuming maciej is okay ith the hash size reduction
Comment 8 Alexey Proskuryakov 2010-01-15 13:34:06 PST
Duplicate of bug 32754/<rdar://problem/7486511>? I'm not sure if anything tracks related UntypedPtrAndBitfield issue.
Comment 9 Gavin Barraclough 2010-01-15 16:44:46 PST
Created attachment 46718 [details]
New implementation for UStringImpl.

Instead, merge the bits into the ref count (ty Alexey for the suggestion!)
No perf degradation, so let's go with this.  Restore the hash to a full 32-bits.
Comment 10 Gavin Barraclough 2010-01-15 16:46:37 PST
Comment on attachment 46704 [details]
The patch

This patch is no longer valid, since we're not going to steal bits from the hash.
Will instead probably do something like UStringImpl, stealing bits from the refCount.
Comment 11 WebKit Review Bot 2010-01-15 16:50:23 PST
Attachment 46718 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/UStringImpl.h:66:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
JavaScriptCore/runtime/UStringImpl.h:134:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/runtime/UStringImpl.h:135:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3
Comment 12 Gavin Barraclough 2010-01-16 19:14:31 PST
Created attachment 46756 [details]
New WebCore::StringImpl patch

New patch for StringImpl, stealing bits from refCount instead of hash.
Comment 13 WebKit Review Bot 2010-01-16 19:16:07 PST
Attachment 46756 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/StringImpl.h:105:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:106:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:180:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3
Comment 14 Darin Adler 2010-01-16 19:28:30 PST
Comment on attachment 46756 [details]
New WebCore::StringImpl patch

> +    OwnPtr<const UChar*> path(fileName.copyWithNullTermination());

This needs to be OwnArrayPtr<const UChar>, not OwnPtr<const UChar*>. Because delete [] and delete without the "[]" are not the same thing.

> +    OwnPtr<const UChar*> m_queryWithNullTermination;

Same here.

> +    const UChar* copyWithNullTermination() const;

This should probably return PassOwnArrayPtr, not a raw pointer. Otherwise it's too easy to get the type wrong for the smart pointer (as above). Unfortunately, there is no PassOwnArrayPtr as of this writing.

Another good possibility would be to have the function take in an OwnArrayPtr<const UChar>& and set it to point to the newly allocated buffer.

> +    static const unsigned s_refCountMask = 0xfffffffe;

I personally prefer capital hex.

review+ assuming you fix the OwnArrayPtr issue.
Comment 15 WebKit Review Bot 2010-01-16 19:33:32 PST
Attachment 46756 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/192656
Comment 16 Gavin Barraclough 2010-01-17 22:58:02 PST
*** Bug 32754 has been marked as a duplicate of this bug. ***
Comment 17 Gavin Barraclough 2010-01-17 23:31:24 PST
JIT changes landed in r53391.
Comment 18 Gavin Barraclough 2010-01-17 23:37:15 PST
UStringImpl changes landed in r53392.
Comment 19 Eric Seidel (no email) 2010-01-18 04:03:41 PST
Looks like r 53392 broke several builds.
Comment 20 Eric Seidel (no email) 2010-01-18 04:09:11 PST
By that, I mean it caused various SVG tests to crash on the builders:
svg/custom/resource-invalidate-on-target-update.svg crashed:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53392%20(9472)/results.html

20 tests crashed:
http://build.webkit.org/results/Windows%20Debug%20(Tests)/r53392%20(8626)/results.html

3 svg tests crashed:
http://build.webkit.org/results/GTK%20Linux%20Release/r53393%20(7754)/results.html
svg/custom/selectSubString.html	stderr
svg/dom/SVGScriptElement/script-load-and-error-events.svg	stderr
svg/dom/SVGScriptElement/script-set-href.svg	stderr

The bot seems to think it was related to this checkin.  I recommend we roll this out.
Comment 21 Eric Seidel (no email) 2010-01-18 04:12:59 PST
The crashes went away for Leopard 3 builds later and then came back again as:

svg/custom/scrolling-embedded-svg-file-image-repaint-problem.html	stderr
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53397%20(9477)/results.html

Looks like flakiness, very possibly related to this checkin since I've never seen this crash before.
Comment 22 Eric Seidel (no email) 2010-01-18 04:17:19 PST
I'm going to roll out r53392 in an effort to re-green the bots.  If it doesn't help, I"m happy to roll it back in.
Comment 23 Csaba Osztrogonác 2010-01-18 04:19:03 PST
r53391 and r53392 rolled out by http://trac.webkit.org/changeset/53400 because of flakey crashes on Leopard, Snow Leopard, Windows, GTK, Qt bots.
Comment 24 Gavin Barraclough 2010-01-18 11:29:31 PST
WebCore::StringImpl changes landed in r53416.
Comment 25 Gavin Barraclough 2010-01-18 11:45:53 PST
WebCore changes need more work on Windows, rolling out in r53418.
Comment 26 Gavin Barraclough 2010-01-18 14:32:11 PST
Created attachment 46851 [details]
WebCore::StringImpl patch take 3, re-introduce String::charactersWithNullTermination() / StringImpl::hasTerminatingNullCharacter()
Comment 27 Gavin Barraclough 2010-01-18 14:46:10 PST
Comment on attachment 46851 [details]
WebCore::StringImpl patch take 3, re-introduce String::charactersWithNullTermination() / StringImpl::hasTerminatingNullCharacter()

Gah! no, this will result in a string's length changing when you access the null terminated form.  Nevermind, I'll go with the simple solution & burn another bit. :-(
Comment 28 Gavin Barraclough 2010-01-18 15:13:38 PST
Created attachment 46858 [details]
take 4.  Simple approach, add a flag back in for HasTerminatingNullCharacter.
Comment 29 WebKit Review Bot 2010-01-18 15:16:38 PST
Attachment 46858 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/StringImpl.h:109:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:110:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:184:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Gavin Barraclough 2010-01-18 18:23:45 PST
WebCore changes relanded in r53447.
Comment 31 Gavin Barraclough 2010-01-19 13:36:47 PST
Created attachment 46945 [details]
New UStringImpl patch, without ancillary refactoring changes.
Comment 32 WebKit Review Bot 2010-01-19 13:39:45 PST
Attachment 46945 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/UStringImpl.h:131:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/runtime/UStringImpl.h:132:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Darin Adler 2010-01-19 15:32:01 PST
Comment on attachment 46945 [details]
New UStringImpl patch, without ancillary refactoring changes.

Damn, it removed my review comments!

There weren’t any important ones. One thing I asked was whether we needed more ALWAYS_INLINE.
Comment 34 Gavin Barraclough 2010-01-20 16:21:19 PST
The webkit patch caused leaks - this is probably also what was causing the JSC patches as originally landed to cause tests crashes on some machines (i.e. they were causing so much leakage DRT actually ran out of memory).

So, reverted in r53575. :-(  *sniff*
Comment 35 Eric Seidel (no email) 2010-02-02 14:17:17 PST
I'm not sure which (both?) of these patches was reverted.  Something tells me this probably doesn't need to be in the http://webkit.org/pending-commit list (by having patches marked r+).
Comment 36 Gavin Barraclough 2010-02-02 14:24:43 PST
Hey Eric,

All patches are currently reverted, but the first one I think is probably good to go – probably didn't need reverting out in the first place, so I'll leave that r+ed & reland soon.

Removed the r+ from the latter two patches, am working on updated patches for these at the mo.

cheers,
G.
Comment 37 Eric Seidel (no email) 2010-02-02 14:26:07 PST
Thanks for the quick reply.
Comment 38 Eric Seidel (no email) 2010-02-08 15:10:54 PST
Comment on attachment 46702 [details]
The patch

Cleared Oliver Hunt's review+ from obsolete attachment 46702 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 39 Eric Seidel (no email) 2010-02-08 15:11:02 PST
Comment on attachment 46718 [details]
New implementation for UStringImpl.

Cleared Oliver Hunt's review+ from obsolete attachment 46718 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 40 Eric Seidel (no email) 2010-02-08 15:11:10 PST
Comment on attachment 46756 [details]
New WebCore::StringImpl patch

Cleared Darin Adler's review+ from obsolete attachment 46756 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 41 Gavin Barraclough 2010-02-12 13:53:12 PST
Created attachment 48663 [details]
New StringImpl patch, also fix bufferIsInternal bug that was being triggered.
Comment 42 WebKit Review Bot 2010-02-12 13:56:44 PST
Attachment 48663 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/StringImpl.h:120:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:121:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/text/StringImpl.h:121:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/platform/text/StringImpl.h:195:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 Sam Weinig 2010-02-12 14:02:42 PST
Comment on attachment 48663 [details]
New StringImpl patch, also fix bufferIsInternal bug that was being triggered.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 54731)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,38 @@
> +2010-02-12  Gavin Barraclough  <barraclough@apple.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=33731
> +        Remove uses of PtrAndFlags from WebCore::StringImpl.
> +
> +        These break the OS X Leaks tool.  Move the management of null-terminated copies
> +        out from StringImpl to String, and use a bits stolen from the refCount to hold the

LIES!

Please fix the change log and add the extra assert in the StringImpl destructor.

Otherwise, r=me.
Comment 44 David Levin 2010-02-12 14:14:46 PST
(In reply to comment #43)
> (From update of attachment 48663 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 54731)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,38 @@
> > +2010-02-12  Gavin Barraclough  <barraclough@apple.com>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        https://bugs.webkit.org/show_bug.cgi?id=33731
> > +        Remove uses of PtrAndFlags from WebCore::StringImpl.
> > +
> > +        These break the OS X Leaks tool.  Move the management of null-terminated copies
> > +        out from StringImpl to String, and use a bits stolen from the refCount to hold the
> 
> LIES!
> 
> Please fix the change log and add the extra assert in the StringImpl
> destructor.
> 
> Otherwise, r=me.

Well, I started reviewing it, but Sam was faster than me. Thanks for doing this (and sorry for breaking leaks).

Something to consider:
> +    static const unsigned s_refCountMask = 0xFFFFFFF0;
What about 
 static const unsigned s_refCountMask = UINT_MAX & ~(s_refCountIncrement - 1)
or just
 static const unsigned s_refCountMask = ~(s_refCountIncrement - 1)
Comment 45 Gavin Barraclough 2010-02-12 14:23:45 PST
New webcore::stringimpl patch landed in r54736.

/me crosses fingers, toes, noses.
Comment 46 Gavin Barraclough 2010-02-12 16:00:56 PST
Landed my previous UStringImpl patch in r54743.

This was r+ from Darin before I marked r-, while debugging an apparent leak.  The leak seems to be well understood now, it seems these patches change UStringImpl / StringImpl objects to be 32 bytes instead of 40, and this in turn triggers the pre existing bug (fixed in the StringImpl patch)  that if string data falls in the subsequent heap block WebCore::StringImpl would confuse this for an internal buffer & fail to free it.
Comment 47 Gavin Barraclough 2010-02-12 19:09:50 PST
JIT patch landed in r54747.
Comment 48 Gavin Barraclough 2010-02-12 20:03:44 PST
Created attachment 48693 [details]
Last patch v0.1! Needs the ChnageLog filling out.
Comment 49 Gavin Barraclough 2010-02-14 17:25:26 PST
Created attachment 48735 [details]
Final Patch!  Refactor StructureTransitionTable back into Structure, remove PtrAndFlags!
Comment 50 WebKit Review Bot 2010-02-14 17:31:40 PST
Attachment 48735 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/Structure.h:249:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/runtime/Structure.h:264:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/runtime/Structure.h:328:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/runtime/Structure.h:340:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/runtime/Structure.h:341:  More than one command on the same line  [whitespace/newline] [4]
Skipping input 'JavaScriptCore/wtf/PtrAndFlags.h': Can't open for reading
Skipping input 'WebCore/ForwardingHeaders/wtf/PtrAndFlags.h': Can't open for reading
Total errors found: 5


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 51 Darin Adler 2010-02-15 12:54:47 PST
Comment on attachment 48735 [details]
Final Patch!  Refactor StructureTransitionTable back into Structure, remove PtrAndFlags!

> +    , m_usingSingleSlot(true)

The naming scheme for booleans where we make them finish a sentence "structure <xxx>" would mean this should be named m_isUsingSingleSlot.

> +        bool structureTransitionTableContains(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
> +        Structure* structureTransitionTableGet(const StructureTransitionTableHash::Key& key, JSCell* specificValue) const
> +        bool structureTransitionTableHasTransition(const StructureTransitionTableHash::Key& key) const
> +        void structureTransitionTableRemove(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
> +        void structureTransitionTableAdd(const StructureTransitionTableHash::Key& key, Structure* structure, JSCell* specificValue)
> +        TransitionTable* structureTransitionTable() const { ASSERT(!m_usingSingleSlot); return m_transitions.m_table; }
> +        void setStructureTransitionTable(TransitionTable* table)

Seems we should name these function members transitionTableXXX since they are members of the Structure class.

It also seems like the header would be a little easier to read if the function bodies were outside the class definition, marked inline. Definitions of functions that are used only inside Structure.cpp can go in that file rather than the header.

r=me
Comment 52 Gavin Barraclough 2010-02-15 15:56:08 PST
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
Sending        JavaScriptCore/bytecode/CodeBlock.h
Sending        JavaScriptCore/runtime/Structure.cpp
Sending        JavaScriptCore/runtime/Structure.h
Sending        JavaScriptCore/runtime/StructureTransitionTable.h
Deleting       JavaScriptCore/wtf/PtrAndFlags.h
Sending        WebCore/ChangeLog
Deleting       WebCore/ForwardingHeaders/wtf/PtrAndFlags.h
Transmitting file data .......
Committed revision 54798.

PtrAndFlags is gone, resolved fixed.

A similar problem is know to exist due to our Rope implementation, but tracking that in a new bug ( https://bugs.webkit.org/show_bug.cgi?id=34964 ).
Comment 53 Joseph Pecoraro 2010-05-07 14:15:48 PDT
The last patch removed PtrAndFlags.h. However, references to the file still exist:

    JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj
    444:			RelativePath="..\..\wtf\PtrAndFlags.h"

    JavaScriptGlue/ForwardingHeaders/wtf/PtrAndFlags.h
    1:#include <JavaScriptCore/PtrAndFlags.h>

Any cause for worry?