I have some hacks to Ref/RefPtr to make it possible to track unbalanced ref()/deref(). The idea is this: * typedef unsigned WTF::RefToken * change the signature of void ref() to WTF::RefToken ref(); * change the signature of void deref() to void defref(WTF::RefToken); * Add RefTracker, which creates new RefToken, and records a stack trace when it does so. RefTracker stores a HashMap<RefToken, std::unique_ptr<StackShot>>. * RefTracker removes its hash map entry when deref() is called with a given token. * For a class whose refs you want to track (e.g. Node), store a RefTracker. * The ref implementation becomes: WTF::RefToken Node::ref() { ++m_refCount; return m_refTracker.trackRef() } void Node::deref(WTF::RefToken token) { m_refTracker.trackDeref(token); --m_refCount; } Then RefPtr<> and Ref<> are changed to store WTF::RefTokens, allowing them to pass tokens between matched ref() and deref(). Then, at some point, you ask the RefTracker to dump the StackShots that remain, which will be the ones for which no token was tracked, or which are unbalanced ref() and deref(). Of course this breaks down with code that does manual ref() and deref(), though callers can do their own RefToken tracking to help pair things up.
Created attachment 341904 [details] Hacky patch
This seems useful. I think the main question is whether it can be done cleanly (preferably with fewer #ifdefs) that doesn't impact performance of builds where it's not enabled.
I think it might be onerous to have to find somewhere to store a token every time you ref/deref something outside of RefPtr/Ref. I believe that Xcode (maybe it’s Instruments) has features that do this quite well for retain and release without intrusive changes to the code that calls retain and release. I think we should explore how those work and whether we can hook up ref/deref to that. But I am certainly not opposed to this on principle. Since we normally use smart pointers and not explicit ref/deref calls. We can just ask people to be even more consistent about always doing that. I would prefer to fix call sites by having them use RefPtr and Ref more rather than by decorating calls to ref and deref with additional macro arguments.
Sam and I looked at what it would take to expose ref/deref to Instruments, and it's not as useful as this. Instruments pairs up ref/deref with some logic that compares stack traces, and does poorly if these are not cleanly nested. I've done this on numerous traces recently, when I proxied ref/deref on Node to a dummy CF object just so that I could leverage Instruments' UI for pairing stacks. It still required lots of manual inspection and error-prone pairing. Conversely, with this token scheme, I was able to track ref/deref on Document and get a SINGLE unmatched ref() stack for a specific layout test that was abandoning a document. I agree that tracking tokens outside of RefPtr and Ref might be laborious, but: a) we can make more helper classes for common patterns b) you can start with the built-in support, and then hack in token tracking for manual refs/derefs as you need them. I bet most classes would only need a few changes to convert all ref/deref to being tracked. And the token tracking is really easy.
(In reply to Darin Adler from comment #3) > I would prefer to fix call > sites by having them use RefPtr and Ref more rather than by decorating calls > to ref and deref with additional macro arguments. Most of the code changes are where classes are implementing ref() and deref(). We could either make macros to declare ref() and deref(), or maybe encode more common patterns in templatized classes like RefCounted<>.
Created attachment 348580 [details] Patch
Attachment 348580 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 14 in 279 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348584 [details] Patch
Attachment 348584 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:40: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 300 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348586 [details] Patch
Attachment 348586 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:40: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 300 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348588 [details] Patch
Attachment 348588 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:40: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 302 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348589 [details] Patch
Attachment 348589 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:40: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 303 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 348589 [details] Patch Attachment 348589 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9045029 Number of test failures exceeded the failure limit.
Created attachment 348592 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 348589 [details] Patch Attachment 348589 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9045045 Number of test failures exceeded the failure limit.
Created attachment 348595 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 348589 [details] Patch Attachment 348589 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9045007 Number of test failures exceeded the failure limit.
Created attachment 348596 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
This patch seems to break the CSS JIT. A selector like ".test.a" no longer matches, but just ".test" or ".a" do. Antti, does the CSS JIT make assumptions about sizeof(Ref<>) or sizeof(RefPtr<>) or sizeof() other classes anywhere?
Created attachment 348605 [details] Patch (tracking enabled)
Attachment 348605 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:40: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 303 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348606 [details] Patch
Created attachment 348608 [details] Patch (tracking disabled)
Attachment 348608 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:40: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 303 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 348605 [details] Patch (tracking enabled) Attachment 348605 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9046636 Number of test failures exceeded the failure limit.
Created attachment 348610 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
<rdar://problem/43925361>
<rdar://problem/43925362>
Comment on attachment 348605 [details] Patch (tracking enabled) Attachment 348605 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9046956 Number of test failures exceeded the failure limit.
Created attachment 348613 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 348605 [details] Patch (tracking enabled) Attachment 348605 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9046919 Number of test failures exceeded the failure limit.
Created attachment 348614 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 348605 [details] Patch (tracking enabled) Attachment 348605 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9048556 New failing tests: fast/selectors/nth-child-of-register-requirement.html fast/selectors/nth-last-child-of-register-requirement.html
Created attachment 348632 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
(In reply to Simon Fraser (smfr) from comment #22) > This patch seems to break the CSS JIT. A selector like ".test.a" no longer > matches, but just ".test" or ".a" do. Antti, does the CSS JIT make > assumptions about sizeof(Ref<>) or sizeof(RefPtr<>) or sizeof() other > classes anywhere? I don't know about size assumptions but at least there is an assumption that you can cast RefPtr<T> to T*. See m_assembler.loadPtr(Assembler::Address(elementAddressRegister, Element::tagQNameMemoryOffset() + QualifiedName::implMemoryOffset()), qualifiedNameImpl); for example.
There might well be a size assumption hiding in all these offset macros and functions.
Yeah, JSC JIT also assumes that Ref<UniquedStringImpl>’s size is the same to UniquedStringImpl*.
Created attachment 348640 [details] Patch (tracking disabled)
Attachment 348640 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefTracking.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1206: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1211: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1216: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1221: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1226: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/xml/XPathGrammar.cpp:1231: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:40: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/webaudio/AudioNode.cpp:474: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 303 files If any of these errors are false positives, please file a bug against check-webkit-style.