Bug 33731 – [Dupe of] Many false leaks in release builds due to PtrAndFlags

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

These break the Leaks tool on OS X.

[Gavin Barraclough]

Created attachment 46700 [details]
The patch

[Gavin Barraclough]

Created attachment 46702 [details]
The patch

[Gavin Barraclough]

Created attachment 46704 [details]
The patch

[Oliver Hunt]

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)

[Oliver Hunt]

Comment on attachment 46704 [details]
The patch

i'd like maciej's opinion on stripping a bit from the string hash

[Oliver Hunt]

Comment on attachment 46700 [details]
The patch

bah, use a #define, otherwise r=me :(

[Oliver Hunt]

Comment on attachment 46704 [details]
The patch

r=me assuming maciej is okay ith the hash size reduction

[Alexey Proskuryakov]

Duplicate of bug 32754/<rdar://problem/7486511>? I'm not sure if anything tracks related UntypedPtrAndBitfield issue.

[Gavin Barraclough]

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.

[Gavin Barraclough]

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.

[WebKit Review Bot]

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

[Gavin Barraclough]

Created attachment 46756 [details]
New WebCore::StringImpl patch

New patch for StringImpl, stealing bits from refCount instead of hash.

[WebKit Review Bot]

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

[Darin Adler]

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.

[WebKit Review Bot]

Attachment 46756 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/192656

[Gavin Barraclough]

*** Bug 32754 has been marked as a duplicate of this bug. ***

[Gavin Barraclough]

JIT changes landed in r53391.

[Gavin Barraclough]

UStringImpl changes landed in r53392.

[Eric Seidel (no email)]

Looks like r 53392 broke several builds.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Csaba Osztrogonác]

r53391 and r53392 rolled out by http://trac.webkit.org/changeset/53400 because of flakey crashes on Leopard, Snow Leopard, Windows, GTK, Qt bots.

[Gavin Barraclough]

WebCore::StringImpl changes landed in r53416.

[Gavin Barraclough]

WebCore changes need more work on Windows, rolling out in r53418.

[Gavin Barraclough]

Created attachment 46851 [details]
WebCore::StringImpl patch take 3, re-introduce String::charactersWithNullTermination() / StringImpl::hasTerminatingNullCharacter()

[Gavin Barraclough]

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

[Gavin Barraclough]

Created attachment 46858 [details]
take 4.  Simple approach, add a flag back in for HasTerminatingNullCharacter.

[WebKit Review Bot]

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.

[Gavin Barraclough]

WebCore changes relanded in r53447.

[Gavin Barraclough]

Created attachment 46945 [details]
New UStringImpl patch, without ancillary refactoring changes.

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Gavin Barraclough]

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*

[Eric Seidel (no email)]

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+).

[Gavin Barraclough]

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.

[Eric Seidel (no email)]

Thanks for the quick reply.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Gavin Barraclough]

Created attachment 48663 [details]
New StringImpl patch, also fix bufferIsInternal bug that was being triggered.

[WebKit Review Bot]

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.

[Sam Weinig]

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 @@
> [phone]  Gavin Barraclough  <[e-mail]>
> +
> +        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.

[David Levin]

(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 @@
> > [phone]  Gavin Barraclough  <[e-mail]>
> > +
> > +        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)

[Gavin Barraclough]

New webcore::stringimpl patch landed in r54736.

/me crosses fingers, toes, noses.

[Gavin Barraclough]

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.

[Gavin Barraclough]

JIT patch landed in r54747.

[Gavin Barraclough]

Created attachment 48693 [details]
Last patch v0.1! Needs the ChnageLog filling out.

[Gavin Barraclough]

Created attachment 48735 [details]
Final Patch!  Refactor StructureTransitionTable back into Structure, remove PtrAndFlags!

[WebKit Review Bot]

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.

[Darin Adler]

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

[Gavin Barraclough]

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

[Joseph Pecoraro]

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?