Add URLSchemeHandler API tests that verify the lack of URLSchemeTask object leaks

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brady Eidson]

Add URLSchemeHandler API tests that verify the lack of URLSchemeTask object leaks

[Brady Eidson]

Followup to https://trac.webkit.org/changeset/220011/webkit

[Brady Eidson]

Created attachment 316683 [details]
Patch

[Brady Eidson]

This patch won't successfully EWS until the patch in https://bugs.webkit.org/show_bug.cgi?id=174950 lands

[Brady Eidson]

(In reply to Brady Eidson from comment #3)
> This patch won't successfully EWS until the patch in
> https://bugs.webkit.org/show_bug.cgi?id=174950 lands

(And it did JUST in time!)

[Alex Christensen]

Comment on attachment 316683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316683&action=review

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKURLSchemeHandler-leaks.mm:47
> +@interface WKWebView (TestingSecrets)
> ++ (size_t)_webURLSchemeTaskInstanceCount;
> ++ (size_t)_apiURLSchemeTaskInstanceCount;
> +@end

Do we need objc selectors for these if they're only used from API tests?  I think we shouldn't put them here.  Instead we could make a new header that accesses these from C++ available only to API tests.

[Brady Eidson]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 316683 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316683&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKURLSchemeHandler-leaks.mm:47
> > +@interface WKWebView (TestingSecrets)
> > ++ (size_t)_webURLSchemeTaskInstanceCount;
> > ++ (size_t)_apiURLSchemeTaskInstanceCount;
> > +@end
> 
> Do we need objc selectors for these if they're only used from API tests?  I
> think we shouldn't put them here.  Instead we could make a new header that
> accesses these from C++ available only to API tests.

Two replies:
1 - They're debug only
2 - The thing you suggest we do... afaik... is not something we do. 

Do you have an example to point to where we reveal WebKit C++ code to TestWebKitAPI?

[Alex Christensen]

We have a C API.

[Brady Eidson]

(In reply to Alex Christensen from comment #7)
> We have a C API.

I mean... sure, we do. That we want to get rid of.

/me shrugs.

[Brady Eidson]

Created attachment 316691 [details]
Patch

[Brady Eidson]

GD, patch rollout caused conflicts with project file changes.

Will revisit soon.

[Brady Eidson]

Created attachment 316701 [details]
Patch

[JF Bastien]

Comment on attachment 316701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316701&action=review

> Source/WTF/wtf/InstanceCounted.h:44
> +    virtual ~InstanceCounted()

I don't understand why virtual is useful here. It's kinda dangerous because is can cause bad deletion if someone holds just the base and deletes the object while the derived type's dtor isn't itself virtual.

> Source/WebKit/UIProcess/API/C/WKTestingSupport.cpp:44
> +    return API::URLSchemeTask::instanceCount();

This would be nicer if the base always declared instanceCount() and it just returned zero in release.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKURLSchemeHandler-leaks.mm:1
> +

Extra space on top.

[Brady Eidson]

(In reply to JF Bastien from comment #12)
> Comment on attachment 316701 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316701&action=review
> 
> > Source/WTF/wtf/InstanceCounted.h:44
> > +    virtual ~InstanceCounted()
> 
> I don't understand why virtual is useful here. It's kinda dangerous because
> is can cause bad deletion if someone holds just the base and deletes the
> object while the derived type's dtor isn't itself virtual.

I might misunderstand what you're saying, as it goes against my understanding of virtual destructors. I thought any virtual method in a base makes any derived method virtual, even if the derivers don't explicitly declare virtual. And I thought this extended to destructors.

e.g.

class Base {
public:
    ~Base() { printf("~Base\n"); }
};

class Derived : public Base {
public:
    ~Derived() { printf("~Derived\n"); }
};

Base* b = new Derived;
delete b;  // Only ~Base is called. ~Derived is *not*

class VBase {
public:
    virtual ~VBase() { printf("~VBase\n"); }
};

class VDerived : public VBase {
public:
    ~VDerived() { printf("~VDerived\n"); } // This destructor is automatically virtual
};

VBase* b = new VDerived;
delete b; // Both ~VDerived and ~VBase are called

---

Just ran that code to confirm my understanding. Am I misunderstanding you, or missing something else here?

> > Source/WebKit/UIProcess/API/C/WKTestingSupport.cpp:44
> > +    return API::URLSchemeTask::instanceCount();
> 
> This would be nicer if the base always declared instanceCount() and it just
> returned zero in release.

That'd probably be fine.

[Darin Adler]

Comment on attachment 316701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316701&action=review

> Source/WTF/wtf/InstanceCounted.h:2
> + * Copyright (C) 2017 Apple Inc.  All rights reserved.

Should only have one space here after the period.

> Source/WTF/wtf/InstanceCounted.h:29
> +// Since it adds runtime overhead, in both managing the count variable and adding a vtable,

We get no benefit from adding a vtable here (see below).

>>> Source/WTF/wtf/InstanceCounted.h:44
>>> +    virtual ~InstanceCounted()
>> 
>> I don't understand why virtual is useful here. It's kinda dangerous because is can cause bad deletion if someone holds just the base and deletes the object while the derived type's dtor isn't itself virtual.
> 
> I might misunderstand what you're saying, as it goes against my understanding of virtual destructors. I thought any virtual method in a base makes any derived method virtual, even if the derivers don't explicitly declare virtual. And I thought this extended to destructors.
> 
> e.g.
> 
> class Base {
> public:
>     ~Base() { printf("~Base\n"); }
> };
> 
> class Derived : public Base {
> public:
>     ~Derived() { printf("~Derived\n"); }
> };
> 
> Base* b = new Derived;
> delete b;  // Only ~Base is called. ~Derived is *not*
> 
> class VBase {
> public:
>     virtual ~VBase() { printf("~VBase\n"); }
> };
> 
> class VDerived : public VBase {
> public:
>     ~VDerived() { printf("~VDerived\n"); } // This destructor is automatically virtual
> };
> 
> VBase* b = new VDerived;
> delete b; // Both ~VDerived and ~VBase are called
> 
> ---
> 
> Just ran that code to confirm my understanding. Am I misunderstanding you, or missing something else here?

Yes, you are correct Brady, the danger JF mentions of bad deletion is not real because if a base class has a virtual destructor the derived classes’ destructors are automatically virtual too.

But I agree with JF that it is not helpful or necessary to make the destructor in this class template be virtual. Instead the destructor should be non-virtual. The destructor should also be protected rather than public.

A virtual destructor here would help us if we wanted to allow people to safely destroy the object that contains this sub-object using an InstanceCounted<X>*; if it’s a virtual destructor then we will properly destroy the larger object, calling the correct destructors.

But we don’t want that. Instead we simply want people to not even try to destroy the object using an InstanceCounted<X>*; we’d like it to be a compilation error. A way to *almost* express that is to make the destructor protected. If it is protected, it can be called implicitly inside derived classes’ destructors and can’t be called by code outside derived classes. It’s not quite right, because sadly there is no simple way to make sure it can’t be called in other places in derived classes’ code, which we would like to forbid if we could.

The rule of thumb for virtual-ness of destructors is generally: If the class is designed to be used polymorphically, it will define at least one virtual function, and then the destructor should be virtual too. The compiler even generates a warning for that case. This class is not designed to be used polymorphically and so it need not have any virtual functions. There are plenty of other examples of that in WebKit.

The constructor should also be protected, for the same reason that the destructor should be. The only public member the class template needs is the instanceCount function; no other members are designed to be used outside the class template, except that the derived classes need to call through to the constructor and destructor in their constructors and destructors.

This class template also should also define protected copy and move constructors that increment the instance count just like the default constructor does, unless we plan to use it only for non-copyable, non-movable classes. If we don’t define copy and move constructors at all, the compiler will generate public ones automatically, and when it does it will generate constructors that don’t increment the instance count. Since a move does not destroy an object, even the move constructor should just bump the instance count by 1.

> Source/WTF/wtf/InstanceCounted.h:53
> +    static std::atomic_size_t s_instances;

s_instances sounds like a collection of instances, not a count of instances. I suggest calling it instanceCount, just like the function.

>>> Source/WebKit/UIProcess/API/C/WKTestingSupport.cpp:44
>>> +    return API::URLSchemeTask::instanceCount();
>> 
>> This would be nicer if the base always declared instanceCount() and it just returned zero in release.
> 
> That'd probably be fine.

Yes, great idea.

[Brady Eidson]

Great!

I had an email thread with JF about string-based lookup of type names, but that's an enhancement to be explored separately.

Wanna get this one out of my tree with that review feedback

[Brady Eidson]

Created attachment 316739 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316739 [details]
Patch

Clearing flags on attachment: 316739

Committed r220049: <http://trac.webkit.org/changeset/220049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33613627>

[Darin Adler]

Comment on attachment 316739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316739&action=review

> Source/WTF/wtf/InstanceCounted.h:26
> +#pragma once

Something I missed in my original review:

This header also needs:

    #include <atomic>

As mentioned in bug 174895 comment 4, it is unnecessary to add any code in WebKit to make the fix testable. Can you explain why you chose to go this route?

[Sam Weinig]

(In reply to mitz from comment #21)
> As mentioned in bug 174895 comment 4, it is unnecessary to add any code in
> WebKit to make the fix testable. Can you explain why you chose to go this
> route?

I'm also curious. I think it is also a bad precedent to set that some things are only testable in debug builds. As far as I know, up until now, we have had testing parity (with perhaps the exception of testing that an assert does not fire) between the build styles, and it seems like a larger conversation should be had to change that.

[Brady Eidson]

(In reply to mitz from comment #21)
> As mentioned in bug 174895 comment 4, it is unnecessary to add any code in
> WebKit to make the fix testable. Can you explain why you chose to go this
> route?

weakptr can only test the API impl object, not the underlying WebKit object.

On that note, I could've skipped the API::URLSchemeTask object here as it's assumed to have 1:1 parity with the WKURLSchemeTask object, but the WebURLSchemeTask object's lifetime is different.

[Brady Eidson]

(In reply to Sam Weinig from comment #22)
> I'm also curious. I think it is also a bad precedent to set that some things
> are only testable in debug builds. As far as I know, up until now, we have
> had testing parity (with perhaps the exception of testing that an assert
> does not fire) between the build styles, and it seems like a larger
> conversation should be had to change that.

Note that the test doesn't behave differently in release builds... it literally doesn't even exist in release builds.

I think it's been bad precedent that we shirk away from testing internals in ways that have a significant impact on the software users use (e.g. leaking internal implementation detail objects that are not visible at the API layer).

I've personally have had an increasing frustration that our regression test policy is easily excepted for quiet testable fixes like this because it's traditionally been a little too hard, or a little extra work that nobody wanted to tackle, etc.

I've also personally observed many other contributors who share this frustration.

This time, the error was so glaring and the effect on WebKit apps so potentially glaring that I decided to take on the work. I find great value in testing internal behaviors like this, and have no qualms about making such a test be in debug-only builds if doing the work in release builds is undesirable.

In my 11 years with the project (Not longer than you but definitely long enough to claim some amount of clout) I've also never heard a discussion about testing parity between build styles. I just don't think it ever comes up in any meaningful manner.

Indeed, layout-tests are still a vast majority of our test coverage and differentiating based on build style there *should never* come up (except, of course, for the ASSERT case you already mentioned).

API tests are a different beast.

[Sam Weinig]

(In reply to Brady Eidson from comment #24)
> (In reply to Sam Weinig from comment #22)
> > I'm also curious. I think it is also a bad precedent to set that some things
> > are only testable in debug builds. As far as I know, up until now, we have
> > had testing parity (with perhaps the exception of testing that an assert
> > does not fire) between the build styles, and it seems like a larger
> > conversation should be had to change that.
> 
> Note that the test doesn't behave differently in release builds... it
> literally doesn't even exist in release builds.

I think that counts as behaving differently.

> 
> I think it's been bad precedent that we shirk away from testing internals in
> ways that have a significant impact on the software users use (e.g. leaking
> internal implementation detail objects that are not visible at the API
> layer).
> 
> I've personally have had an increasing frustration that our regression test
> policy is easily excepted for quiet testable fixes like this because it's
> traditionally been a little too hard, or a little extra work that nobody
> wanted to tackle, etc.
> 
> I've also personally observed many other contributors who share this
> frustration.
> 

I don't disagree with any of this, and as the author of the increasingly misnamed TestWebKitAPI and of many of the non API / internal tests I think I have some bonafides in the area :).

> This time, the error was so glaring and the effect on WebKit apps so
> potentially glaring that I decided to take on the work. I find great value
> in testing internal behaviors like this, and have no qualms about making
> such a test be in debug-only builds if doing the work in release builds is
> undesirable.
> 
> In my 11 years with the project (Not longer than you but definitely long
> enough to claim some amount of clout) I've also never heard a discussion
> about testing parity between build styles. I just don't think it ever comes
> up in any meaningful manner.

It hasn't really been something that needed to come up. We have talked a lot about parity in other places where parity started to diverge, usually with the emphasis on parity being good.

> Indeed, layout-tests are still a vast majority of our test coverage and
> differentiating based on build style there *should never* come up (except,
> of course, for the ASSERT case you already mentioned).
> 
> API tests are a different beast.

I'm not really sure they should be. But maybe. As I said, I think this deserves wider discussion. Maybe Debug/Release shouldn't be the boundary, but rather Debug/Release vs. Production. Better yet, I've found that in most cases, though perhaps not all, you can find a way to test what you want without resorting to expensive internal changes.

In this case, for instance, you want to know about the lifetime of two objects: API::URLSchemeTask and WebKit::WebURLSchemeTask. For API::URLSchemeTask, this is easy, as, it's castable to a WKURLSchemeTaskImpl, and you can use an Objective-C weak pointer. Given it has a strong pointer to the WebURLSchemeTask, this should probably be enough to prove the bug is fixed. But, we can probably take it further. There really isn't a good reason for WebKit::WebURLSchemeTask to exist at all. Instead, it could be folded into API::URLSchemeTask (avoiding the extra allocation and ref counting) and then the objective-c weakpointer counting will test everything.

[JF Bastien]

Oh wow what I wrote makes no sense at all! Apologies for this :)

I had something in mind, which I failed to express, and which doesn't really matter either (holding a *sibling* pointer when one is virtual, and other isn't). That's a leak with or without virtual, so ignore me.

[Brady Eidson]

(In reply to Sam Weinig from comment #25)
> (In reply to Brady Eidson from comment #24)
> > (In reply to Sam Weinig from comment #22)
> > 
> > In my 11 years with the project (Not longer than you but definitely long
> > enough to claim some amount of clout) I've also never heard a discussion
> > about testing parity between build styles. I just don't think it ever comes
> > up in any meaningful manner.
> 
> It hasn't really been something that needed to come up. We have talked a lot
> about parity in other places where parity started to diverge, usually with
> the emphasis on parity being good.

I've never heard such conversations. I'm curious what some of the examples are for parity being a good end goal in and of itself.

> 
> > Indeed, layout-tests are still a vast majority of our test coverage and
> > differentiating based on build style there *should never* come up (except,
> > of course, for the ASSERT case you already mentioned).
> > 
> > API tests are a different beast.
> 
> I'm not really sure they should be. But maybe. As I said, I think this
> deserves wider discussion. Maybe Debug/Release shouldn't be the boundary,
> but rather Debug/Release vs. Production. 

If I had any confidence that we did perf testing only on production builds and not release builds, I think this would be a great approach.

I'm still not sure what the driving motivation is behind "tests should be targeted to both debug and release, not only one of them"

> In this case, for instance, you want to know about the lifetime of two
> objects: API::URLSchemeTask and WebKit::WebURLSchemeTask. For
> API::URLSchemeTask, this is easy, as, it's castable to a
> WKURLSchemeTaskImpl, and you can use an Objective-C weak pointer. 

Is our solution, then, to rely on Objective-C weak pointers for leak tracking API types?
If so, where does that leave non-Obj-C platforms?

> Given it has a strong pointer to the WebURLSchemeTask, this should probably be enough
> to prove the bug is fixed. But, we can probably take it further. There
> really isn't a good reason for WebKit::WebURLSchemeTask to exist at all.
> Instead, it could be folded into API::URLSchemeTask (avoiding the extra
> allocation and ref counting) and then the objective-c weakpointer counting
> will test everything.

In this case, this it probably right.

In cases I have planned to apply this technique once I have some down time, this is definitely wrong. There's a lot of past, present, and potential leaks of internal-only objects in the Database and Storage processes where leaking could have expensive ramifications. No API tricks will help us there.

Short of hearing reasons why build-style parity is actually a worthy goal, I look at this from a different perspective:
Would it be great to track it in all build styles without cost? Sure.
But is tracking it in one build style better than tracking it in zero build styles? Absolutely.
Does the code make WebKit worse? I don't think so.

[Sam Weinig]

(In reply to Brady Eidson from comment #27)
> (In reply to Sam Weinig from comment #25)
> > (In reply to Brady Eidson from comment #24)
> > > (In reply to Sam Weinig from comment #22)
> > > 
> > > In my 11 years with the project (Not longer than you but definitely long
> > > enough to claim some amount of clout) I've also never heard a discussion
> > > about testing parity between build styles. I just don't think it ever comes
> > > up in any meaningful manner.
> > 
> > It hasn't really been something that needed to come up. We have talked a lot
> > about parity in other places where parity started to diverge, usually with
> > the emphasis on parity being good.
> 
> I've never heard such conversations. I'm curious what some of the examples
> are for parity being a good end goal in and of itself.
> 

Parity is helpful as it reduces the cognitive burden on contributors. Anytime we have differences, it's a chance for someone to make a mistake or find themselves confused by results.

> > 
> > > Indeed, layout-tests are still a vast majority of our test coverage and
> > > differentiating based on build style there *should never* come up (except,
> > > of course, for the ASSERT case you already mentioned).
> > > 
> > > API tests are a different beast.
> > 
> > I'm not really sure they should be. But maybe. As I said, I think this
> > deserves wider discussion. Maybe Debug/Release shouldn't be the boundary,
> > but rather Debug/Release vs. Production. 
> 
> If I had any confidence that we did perf testing only on production builds
> and not release builds, I think this would be a great approach.
> 
> I'm still not sure what the driving motivation is behind "tests should be
> targeted to both debug and release, not only one of them"

The driving motivation is that you have created a new point of divergence. 

> 
> > In this case, for instance, you want to know about the lifetime of two
> > objects: API::URLSchemeTask and WebKit::WebURLSchemeTask. For
> > API::URLSchemeTask, this is easy, as, it's castable to a
> > WKURLSchemeTaskImpl, and you can use an Objective-C weak pointer. 
> 
> Is our solution, then, to rely on Objective-C weak pointers for leak
> tracking API types?
> If so, where does that leave non-Obj-C platforms?

This is a much broader question. I would argue your solution does not scale to tackle this problem either. Will you add new API entry points for each object you want to specially track?  Our traditional approach to leak tracking is to have dedicated leaks bots, that use the 'leaks' program to find leaks. 

If we wanted something more targeted, the solution I would propose for C++ types is DYLD_INTERPOSE based shimming. As we already make use of swizzling for Objective-C types in the API tests, this seems like a reasonable approach. It applies minimal impact to the code base, and is supported in both debug and release modes.

> 
> > Given it has a strong pointer to the WebURLSchemeTask, this should probably be enough
> > to prove the bug is fixed. But, we can probably take it further. There
> > really isn't a good reason for WebKit::WebURLSchemeTask to exist at all.
> > Instead, it could be folded into API::URLSchemeTask (avoiding the extra
> > allocation and ref counting) and then the objective-c weakpointer counting
> > will test everything.
> 
> In this case, this it probably right.
> 
> In cases I have planned to apply this technique once I have some down time,
> this is definitely wrong. There's a lot of past, present, and potential
> leaks of internal-only objects in the Database and Storage processes where
> leaking could have expensive ramifications. No API tricks will help us there.
> 
> Short of hearing reasons why build-style parity is actually a worthy goal, I
> look at this from a different perspective:
> Would it be great to track it in all build styles without cost? Sure.
> But is tracking it in one build style better than tracking it in zero build
> styles? Absolutely.
> Does the code make WebKit worse? I don't think so.

I think it makes the WebKit code harder to develop for and alternatives that don't create this burden exist. 

I think given I have provided an alternate way to handle this case, it would make sense to revert it, and do the variant that does not create this issue. When it comes to testing leaks for other objects, let's have that discussion then, and we can look at the alternatives I have proposed.

[Alexey Proskuryakov]

This test is not working on iOS, please see bug 174984.

[Brady Eidson]

(In reply to Sam Weinig from comment #28)
> 
> Parity is helpful as it reduces the cognitive burden on contributors.
> Anytime we have differences, it's a chance for someone to make a mistake or
> find themselves confused by results.

I think it'd be useful to stop speaking in theoreticals.

Does this specific addition to the WebKit code base increase cognitive burden for you in a concrete way?
If so, can you describe the concrete way in which it does?

> > > > Indeed, layout-tests are still a vast majority of our test coverage and
> > > > differentiating based on build style there *should never* come up (except,
> > > > of course, for the ASSERT case you already mentioned).
> > > > 
> > > > API tests are a different beast.
> > > 
> > > I'm not really sure they should be. But maybe. As I said, I think this
> > > deserves wider discussion. Maybe Debug/Release shouldn't be the boundary,
> > > but rather Debug/Release vs. Production. 
> > 
> > If I had any confidence that we did perf testing only on production builds
> > and not release builds, I think this would be a great approach.
> > 
> > I'm still not sure what the driving motivation is behind "tests should be
> > targeted to both debug and release, not only one of them"
> 
> The driving motivation is that you have created a new point of divergence.

I'm asking "why is keeping this type of divergence out a driving motivation" and you respond with "the driving motivation is that you've created a point of divergence."

I... am not convinced?

> > Is our solution, then, to rely on Objective-C weak pointers for leak
> > tracking API types?
> > If so, where does that leave non-Obj-C platforms?
> 
> This is a much broader question. I would argue your solution does not scale
> to tackle this problem either. Will you add new API entry points for each
> object you want to specially track? 

Actually, absolutely not. On two notes:
1 - I'd never add API as opposed to SPI
2 - I've been talking with JF on how to have a single API entry point handle all cases automatically.

> Our traditional approach to leak
> tracking is to have dedicated leaks bots, that use the 'leaks' program to
> find leaks. 

Which are slow and so buggy that they aren't watched. It seems more efficient to make a targeted regression test when resolving a targeted regression.

> If we wanted something more targeted, the solution I would propose for C++
> types is DYLD_INTERPOSE based shimming. As we already make use of swizzling
> for Objective-C types in the API tests, this seems like a reasonable
> approach. It applies minimal impact to the code base, and is supported in
> both debug and release modes.

Again, I'm hearing it said that "it's better to not having more extensive testing in one of the build styles" as a given, when it is not a given.

Internally in the office, we've literally already used InstanceCounted to quickly explore a different non-API-exposed leak that the system tools could not track.
It took 3 lines of code to quickly disprove a theory that could not have quickly been disproven in another way.

Also, while exploring it, it became obvious that for the current use cases (URLSchemeTask in this patch, and NetworkDataTaskCocoa in the exploration today), having the code on in a release build would not at all be a burden.

> I think given I have provided an alternate way to handle this case, it would
> make sense to revert it, and do the variant that does not create this issue.

I oppose the notion that this code makes WebKit worse, and reject the unilateral decision to remove it.

[Sam Weinig]

(In reply to Brady Eidson from comment #30)
> (In reply to Sam Weinig from comment #28)
> > 
> > Parity is helpful as it reduces the cognitive burden on contributors.
> > Anytime we have differences, it's a chance for someone to make a mistake or
> > find themselves confused by results.
> 
> I think it'd be useful to stop speaking in theoreticals.
> 
> Does this specific addition to the WebKit code base increase cognitive
> burden for you in a concrete way?
> If so, can you describe the concrete way in which it does?
> 
> > > > > Indeed, layout-tests are still a vast majority of our test coverage and
> > > > > differentiating based on build style there *should never* come up (except,
> > > > > of course, for the ASSERT case you already mentioned).
> > > > > 
> > > > > API tests are a different beast.
> > > > 
> > > > I'm not really sure they should be. But maybe. As I said, I think this
> > > > deserves wider discussion. Maybe Debug/Release shouldn't be the boundary,
> > > > but rather Debug/Release vs. Production. 
> > > 
> > > If I had any confidence that we did perf testing only on production builds
> > > and not release builds, I think this would be a great approach.
> > > 
> > > I'm still not sure what the driving motivation is behind "tests should be
> > > targeted to both debug and release, not only one of them"
> > 
> > The driving motivation is that you have created a new point of divergence.
> 
> I'm asking "why is keeping this type of divergence out a driving motivation"
> and you respond with "the driving motivation is that you've created a point
> of divergence."
> 
> I... am not convinced?
> 
> > > Is our solution, then, to rely on Objective-C weak pointers for leak
> > > tracking API types?
> > > If so, where does that leave non-Obj-C platforms?
> > 
> > This is a much broader question. I would argue your solution does not scale
> > to tackle this problem either. Will you add new API entry points for each
> > object you want to specially track? 
> 
> Actually, absolutely not. On two notes:
> 1 - I'd never add API as opposed to SPI
> 2 - I've been talking with JF on how to have a single API entry point handle
> all cases automatically.
> 
> > Our traditional approach to leak
> > tracking is to have dedicated leaks bots, that use the 'leaks' program to
> > find leaks. 
> 
> Which are slow and so buggy that they aren't watched. It seems more
> efficient to make a targeted regression test when resolving a targeted
> regression.
> 
> > If we wanted something more targeted, the solution I would propose for C++
> > types is DYLD_INTERPOSE based shimming. As we already make use of swizzling
> > for Objective-C types in the API tests, this seems like a reasonable
> > approach. It applies minimal impact to the code base, and is supported in
> > both debug and release modes.
> 
> Again, I'm hearing it said that "it's better to not having more extensive
> testing in one of the build styles" as a given, when it is not a given.
> 
> Internally in the office, we've literally already used InstanceCounted to
> quickly explore a different non-API-exposed leak that the system tools could
> not track.
> It took 3 lines of code to quickly disprove a theory that could not have
> quickly been disproven in another way.
> 
> Also, while exploring it, it became obvious that for the current use cases
> (URLSchemeTask in this patch, and NetworkDataTaskCocoa in the exploration
> today), having the code on in a release build would not at all be a burden.
> 
> > I think given I have provided an alternate way to handle this case, it would
> > make sense to revert it, and do the variant that does not create this issue.
> 
> I oppose the notion that this code makes WebKit worse, and reject the
> unilateral decision to remove it.

I have repeatedly suggested this be taken to a broader setting for discussion (see comments 22 and 25), and don't think that review feedback should be considered "unilateral" decision making. 

I am disappointed in how this conversion went, and wish I knew how I could have made it more productive. I provided concrete suggestions for this bug on how you could achieve the same goal with less code and without violating a concept I believe in, even if you don't.