After the build: https://build.webkit.org/builders/Apple%20Sierra%20%28Leaks%29/builds/2321 The number of reported leaks on the Sierra leaks tester went from 9240 to 235983 according to the archive: https://build.webkit.org/builders/Apple%20Sierra%20%28Leaks%29?numbuilds=200
I'll double check but my cleanup patch is unlikely to be the culprit. Phil should probably take a look.
Also likely to have the same root cause as https://bugs.webkit.org/show_bug.cgi?id=175082
Bug 175082 was fixed, but we still have all those leaks as of r220178.
<rdar://problem/33693778>
I guess it has to be due to https://trac.webkit.org/r219897 (GC should be fine with trading blocks between destructor and non-destructor blocks)
(In reply to Alexey Proskuryakov from comment #5) > I guess it has to be due to https://trac.webkit.org/r219897 (GC should be > fine with trading blocks between destructor and non-destructor blocks) I think so too. I'm looking at this now. I already tried reproducing obvious things. It appears that destructors are still called exactly like they should be in simple tests. I'm currently waiting to run the full tests...
I can repro this by just running fast/events/gc-freeze-with-attribute-listeners.html. In that test, the entire leak is SparseArrayValueMap-related. It looks like some destructor is not running.
This is so strange! It looks as if all of the leak is that we're not GCing at the end, and `leaks` thinks that everything we allocated is a leak (because it kinda is, since we didn't GC).
I just ran into a bug where run-webkit-tests does not clear out the leaks files from previous runs. I was getting totally nonsensical results. Now I'm running with "rm -rf WebKitBuild/Release/layout-test-results" before each test run.
When I run locally, I can get a massive amount of leaks if I can get tests to timeout. This is because the leaks tool works best when we are quiet. If you run leaks at the moment that you are killing the process due to timeout, then things get very confusing. On the leaks bot, there are many timeouts. I wonder if this is the issue.
(In reply to Filip Pizlo from comment #10) > When I run locally, I can get a massive amount of leaks if I can get tests > to timeout. This is because the leaks tool works best when we are quiet. > If you run leaks at the moment that you are killing the process due to > timeout, then things get very confusing. > > On the leaks bot, there are many timeouts. > > I wonder if this is the issue. No, there were many timeouts both before and after the regression.
I think I'm onto it. workers/bomb.html has a massive leak. If you turn off timeout, it leaks ~50MB. It leaks less than that on the bots only because it times out. Crazy!
A huge chunk of the leak was that we would measure leaks while VM destruction was in progress. This meant that as the Heap::lastChanceToFinalize() proceeds through the object graph in basically a random order and destroys objects, at some point we will have something that looks like a massive leak because lastChanceToFinalize() will have cut off some subgraphs from the roots. I fixed this issue by making sure that DumpRenderTree waits for VM shutdown to complete before going to the next test. Once I did that, I could easily reproduce the actual leak. Also, if I disable the ability to trade destructor blocks, the leak goes away. Still for why...
Created attachment 317353 [details] the patch Maybe this'll do it.
Created attachment 317354 [details] the patch
Created attachment 317355 [details] the patch Fix debug build
Comment on attachment 317355 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=317355&action=review r=me assuming perf is clean > Source/WTF/ChangeLog:9 > + Adds a classic ReadWriteLock class. I wrote my own because I can never remember if the pthread one is > + guaranted to bias in favor of writers or not. the best rationale :P > Source/WTF/wtf/Condition.h:177 > Atomic<bool> m_hasWaiters; Any idea why this was protected in the first place? no matter, not relevant to this patch, just curious
Landed in https://trac.webkit.org/changeset/220322/webkit
(In reply to Oliver Hunt from comment #17) > Comment on attachment 317355 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317355&action=review > > r=me assuming perf is clean > > > Source/WTF/ChangeLog:9 > > + Adds a classic ReadWriteLock class. I wrote my own because I can never remember if the pthread one is > > + guaranted to bias in favor of writers or not. > > the best rationale :P > > > Source/WTF/wtf/Condition.h:177 > > Atomic<bool> m_hasWaiters; > > Any idea why this was protected in the first place? no matter, not relevant > to this patch, just curious Because there was no construct() function, so Condition's constructor had to set ConditionBase::m_hasWaiters directly.
Down to 5540 leaks on https://build.webkit.org/builders/Apple%20Sierra%20%28Leaks%29?numbuilds=200 as of build 2498, the first one to have this change. Woohoo!!!