Bug 174895

Summary: WKURLSchemeTaskImpl instances are being abandoned (except if the task is stopped)
Product: WebKit Reporter: mitz
Component: WebKit APIAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, joepeck, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test project
none
Patch
none
Patch none

Description mitz 2017-07-27 08:53:42 PDT
Created attachment 316550 [details]
Test project

WKURLSchemeTaskImpl instances are being abandoned whenever the task completes without being stopped.

To reproduce: build and run the attached program, click the Load button a few times, then click the End button to get the WKWebView deallocated (not that it should matter). Then use a heap inspection tool (Xcode’s memory graph viewer or just "heap TestSchemeHandler -addresses WKURLSchemeTaskImpl") to see if there are any remaining WKURLSchemeTaskImpl instances.

Note: The instances don’t seem to be leaked, just abandoned. The API::URLSchemeTask has a strong reference to its WebURLSchemeTask which holds a reference to the WebURLSchemeHandler, which holds references to all its unstopped tasks (I think that’s the cycle that should get broken, but currently only gets broken upon stop, not upon completion).
Comment 1 Radar WebKit Bug Importer 2017-07-27 08:54:32 PDT
<rdar://problem/33568276>
Comment 2 Brady Eidson 2017-07-27 22:58:08 PDT
Great test project, thanks!

Since writing an automated regression test for this would either:
1 - Be impossible
2 - Require SPI specific to getting heap counts just for testing, which is a practice that this bug reporter doesn't agree with in its current form

...I verified this upcoming fix manually.
Comment 3 Brady Eidson 2017-07-27 22:58:51 PDT
(In reply to Brady Eidson from comment #2)
> Great test project, thanks!
> 
> Since writing an automated regression test for this would either:
> 1 - Be impossible
> 2 - Require SPI specific to getting heap counts just for testing, which is a
> practice that this bug reporter doesn't agree with in its current form
> 
> ...I verified this upcoming fix manually.

(Both by checking heaps and by adding some logging to an API::URLSchemeTask destructor)
Comment 4 mitz 2017-07-27 23:00:52 PDT
(In reply to Brady Eidson from comment #2)
> Great test project, thanks!
> 
> Since writing an automated regression test for this would either:
> 1 - Be impossible
> 2 - Require SPI specific to getting heap counts just for testing, which is a
> practice that this bug reporter doesn't agree with in its current form

It’s neither. It should be testable using an Objective-C weak pointer to the WKURLSchemeTaskImpl. We may already have a test along those lines in TestWebKitAPI.
Comment 5 Brady Eidson 2017-07-27 23:07:47 PDT
Created attachment 316622 [details]
Patch
Comment 6 mitz 2017-07-27 23:14:01 PDT
Comment on attachment 316622 [details]
Patch

Are there other possible scenarios where this bug can happen? For example, if a request has not completed and one of these things happens, will the task be stopped?

1. The web view is closed and deallocated
2. The Web Content process crashes
3. The Networking process crashes
Comment 7 Brady Eidson 2017-07-27 23:36:50 PDT
(In reply to mitz from comment #6)
> Comment on attachment 316622 [details]
> Patch
> 
> Are there other possible scenarios where this bug can happen? 
> For example, if a request has not completed and one of these things happens, will the
> task be stopped?
> 
> 1. The web view is closed and deallocated

Yes, the task is stopped.

> 2. The Web Content process crashes
> 3. The Networking process crashes

Let's see!
Comment 8 Brady Eidson 2017-07-27 23:39:22 PDT
(In reply to Brady Eidson from comment #7)
> (In reply to mitz from comment #6)
> 
> > 2. The Web Content process crashes
> > 3. The Networking process crashes
> 
> Let's see!

For both of these cases, no.
Comment 9 mitz 2017-07-27 23:40:57 PDT
(In reply to Brady Eidson from comment #8)
> (In reply to Brady Eidson from comment #7)
> > (In reply to mitz from comment #6)
> > 
> > > 2. The Web Content process crashes
> > > 3. The Networking process crashes
> > 
> > Let's see!
> 
> For both of these cases, no.

These exceptional cases don’t need to be addressed as part of this bug, but they should be fixed.
Comment 10 Brady Eidson 2017-07-28 09:41:15 PDT
(In reply to Brady Eidson from comment #7)
> (In reply to mitz from comment #6)
> > 
> > 1. The web view is closed and deallocated
> 
> Yes, the task is stopped.

This was wrong because the test app explicitly called stopLoading.

Removing that call reintroduces the leak.

Exploring now.
Comment 11 Brady Eidson 2017-07-28 09:45:21 PDT
(In reply to Brady Eidson from comment #10)
> (In reply to Brady Eidson from comment #7)
> > (In reply to mitz from comment #6)
> > > 
> > > 1. The web view is closed and deallocated
> > 
> > Yes, the task is stopped.
> 
> This was wrong because the test app explicitly called stopLoading.
> 
> Removing that call reintroduces the leak.
> 
> Exploring now.

When the WKWebView is torn down on the UIProcess side, it tells the WebProcess side.

In the WebProcess, it does *try* to stop these loaders, but it messages back to the WebPageProxy, which is already torn down (see previous step) so it never gets the message.
Comment 12 Brady Eidson 2017-07-28 10:52:14 PDT
Created attachment 316647 [details]
Patch
Comment 13 Brady Eidson 2017-07-28 11:32:14 PDT
In person we've discussed a few testing strategies for this, and have some ideas.

Going to land the patch as-is, and followup with that.
Comment 14 WebKit Commit Bot 2017-07-28 13:03:46 PDT
Comment on attachment 316647 [details]
Patch

Clearing flags on attachment: 316647

Committed r220011: <http://trac.webkit.org/changeset/220011>
Comment 15 WebKit Commit Bot 2017-07-28 13:03:48 PDT
All reviewed patches have been landed.  Closing bug.