Bug 175083 – REGRESSION (r219895-219897): Number of leaks on Open Source went from 9240 to 235983 and is now at 302372

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Matt Lewis]

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

[Chris Dumez]

I'll double check but my cleanup patch is unlikely to be the culprit. Phil should probably take a look.

[Filip Pizlo]

Also likely to have the same root cause as https://bugs.webkit.org/show_bug.cgi?id=175082

[Alexey Proskuryakov]

Bug 175082 was fixed, but we still have all those leaks as of r220178.

[Radar WebKit Bug Importer]

<rdar://problem/33693778>

[Alexey Proskuryakov]

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)

[Filip Pizlo]

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

[Filip Pizlo]

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.

[Filip Pizlo]

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

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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

[Filip Pizlo]

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!

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 317353 [details]
the patch

Maybe this'll do it.

[Filip Pizlo]

Created attachment 317354 [details]
the patch

[Filip Pizlo]

Created attachment 317355 [details]
the patch

Fix debug build

[Oliver Hunt]

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

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/220322/webkit

[Filip Pizlo]

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

[Filip Pizlo]

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!!!