Bug 28412 – Leak of WebCore::XMLHttpRequest object during layout tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

Created attachment 35001 [details]
Make XHR leak more obvious

While looking at the output of 'run-webkit-tests --leaks' over the last few days I've occasionally noticed instances of WebCore::XMLHttpRequest being leaked.  I wrote a simple patch that uses RefCountedLeakCounter to make it more obvious when this sort of leak is occurring, and narrowed the leaks down to two tests that are run in sequence.  The following command leaks a single XMLHttpRequest instance every time:

./WebKitTools/Scripts/run-webkit-tests --leaks LayoutTests/http/tests/xmlhttprequest/{xhr-onunload,xml-encoding}.html

With the attached patch applied you'll see:
LEAK: 1 XMLHttpRequest

in the output.

[Mark Rowe (bdash)]

I poked at this briefly and it seemed that the XHR instance gained an extra ref during a call to setPendingActivity which was never balanced out.

[Mark Rowe (bdash)]

<rdar://problem/7149500>

[Alexey Proskuryakov]

This regressed in <http://trac.webkit.org/changeset/47291>, where a call to setPendingActivity() was moved to before ThreadableLoader::create().

Even worse, nonCacheRequestInFlight() call was lost in the refactoring.

[Aaron Boodman]

Whoops, I will this.

[Alexey Proskuryakov]

Created attachment 35073 [details]
Unfinished fix

OK, here's what I already have - unfinished and untested. This also fixed a check before setReportUploadProgress() (which is only used by Chromium afaik).

[Aaron Boodman]

Created attachment 35095 [details]
Combination of previous two patches, plus ChangeLogs

I verified this fixes the leak. Also, reran all LayoutTests, no regressions. And it looks correct, so not sure what else I can add.

[Maciej Stachowiak]

Comment on attachment 35095 [details]
Combination of previous two patches, plus ChangeLogs

r=me

[Mark Rowe (bdash)]

Landed in r47478.

[Eric Seidel (no email)]

Comment on attachment 35095 [details]
Combination of previous two patches, plus ChangeLogs

Rejecting patch 35095 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=35095 from bug 28412 failed to download and apply.

[Alexey Proskuryakov]

+            // For now we should only balance the nonCached request count for main-thread XHRs and not
+            // Worker XHRs, as the Cache is not thread-safe.
+            // This will become irrelevant after https://bugs.webkit.org/show_bug.cgi?id=27165 is resolved.

By the way, it seems that moving this and related code to DocumentThreadableLoader could be a nice improvement while we wait for bug 27165.