ASSIGNED 186269
Make it possible to track unbalanced ref()/deref() 
https://bugs.webkit.org/show_bug.cgi?id=186269
Summary Make it possible to track unbalanced ref()/deref()
Simon Fraser (smfr)
Reported 2018-06-04 08:21:50 PDT
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.
Attachments
Hacky patch (212.81 KB, patch)
2018-06-04 08:23 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (277.78 KB, patch)
2018-08-30 19:08 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (294.70 KB, patch)
2018-08-30 19:34 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (294.87 KB, patch)
2018-08-30 19:54 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (300.74 KB, patch)
2018-08-30 20:19 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (301.66 KB, patch)
2018-08-30 20:26 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-sierra (1.79 MB, application/zip)
2018-08-30 21:26 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (1.85 MB, application/zip)
2018-08-30 21:33 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews117 for mac-sierra (1.53 MB, application/zip)
2018-08-30 21:34 PDT, EWS Watchlist 		no flags
Details
Patch (tracking enabled) (299.93 KB, patch)
2018-08-30 21:56 PDT, Simon Fraser (smfr) 		ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Patch (299.93 KB, patch)
2018-08-30 23:00 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (tracking disabled) (299.85 KB, patch)
2018-08-30 23:08 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews102 for mac-sierra (911.01 KB, application/zip)
2018-08-30 23:25 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (1.93 MB, application/zip)
2018-08-31 00:00 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews115 for mac-sierra (1.55 MB, application/zip)
2018-08-31 00:01 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews201 for win-future (12.78 MB, application/zip)
2018-08-31 04:08 PDT, EWS Watchlist 		no flags
Details
Patch (tracking disabled) (299.89 KB, patch)
2018-08-31 08:26 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2018-06-04 08:23:57 PDT
Created attachment 341904 [details] Hacky patch
Simon Fraser (smfr)
Comment 2 2018-06-04 08:25:26 PDT
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.
Darin Adler
Comment 3 2018-06-04 21:47:34 PDT
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.
Simon Fraser (smfr)
Comment 4 2018-06-04 22:33:04 PDT
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.
Simon Fraser (smfr)
Comment 5 2018-06-04 22:35:31 PDT
(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<>.
Simon Fraser (smfr)
Comment 6 2018-08-30 19:08:21 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2018-08-30 19:11:58 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 8 2018-08-30 19:34:54 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 9 2018-08-30 19:37:02 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 10 2018-08-30 19:54:08 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 11 2018-08-30 19:58:01 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 12 2018-08-30 20:19:34 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 13 2018-08-30 20:22:24 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 14 2018-08-30 20:26:59 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 15 2018-08-30 20:30:43 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 16 2018-08-30 21:26:54 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 17 2018-08-30 21:26:56 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 18 2018-08-30 21:33:48 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 19 2018-08-30 21:33:49 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 20 2018-08-30 21:34:09 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 21 2018-08-30 21:34:11 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 22 2018-08-30 21:54:06 PDT
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?
Simon Fraser (smfr)
Comment 23 2018-08-30 21:56:00 PDT
Created attachment 348605 [details] Patch (tracking enabled)
EWS Watchlist
Comment 24 2018-08-30 21:59:11 PDT Comment hidden (obsolete)
Simon Fraser (smfr)
Comment 25 2018-08-30 23:00:29 PDT
Created attachment 348606 [details] Patch
Simon Fraser (smfr)
Comment 26 2018-08-30 23:08:53 PDT
Created attachment 348608 [details] Patch (tracking disabled)
EWS Watchlist
Comment 27 2018-08-30 23:11:22 PDT
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.
EWS Watchlist
Comment 28 2018-08-30 23:25:19 PDT
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.
EWS Watchlist
Comment 29 2018-08-30 23:25:21 PDT
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
Radar WebKit Bug Importer
Comment 30 2018-08-30 23:25:44 PDT
<rdar://problem/43925361>
Radar WebKit Bug Importer
Comment 31 2018-08-30 23:25:46 PDT
<rdar://problem/43925362>
EWS Watchlist
Comment 32 2018-08-31 00:00:57 PDT
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.
EWS Watchlist
Comment 33 2018-08-31 00:00:59 PDT
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
EWS Watchlist
Comment 34 2018-08-31 00:01:48 PDT
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.
EWS Watchlist
Comment 35 2018-08-31 00:01:51 PDT
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
EWS Watchlist
Comment 36 2018-08-31 04:08:19 PDT
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
EWS Watchlist
Comment 37 2018-08-31 04:08:31 PDT
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
Antti Koivisto
Comment 38 2018-08-31 05:16:08 PDT
(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.
Antti Koivisto
Comment 39 2018-08-31 05:18:23 PDT
There might well be a size assumption hiding in all these offset macros and functions.
Yusuke Suzuki
Comment 40 2018-08-31 05:53:07 PDT
Yeah, JSC JIT also assumes that Ref<UniquedStringImpl>’s size is the same to UniquedStringImpl*.
Simon Fraser (smfr)
Comment 41 2018-08-31 08:26:06 PDT
Created attachment 348640 [details] Patch (tracking disabled)
EWS Watchlist
Comment 42 2018-08-31 08:28:50 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.