Bug 174895 – WKURLSchemeTaskImpl instances are being abandoned (except if the task is stopped)

• Component: WebKit API
• Status: RESOLVED FIXED

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

[Radar WebKit Bug Importer]

<rdar://problem/33568276>

[Brady Eidson]

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.

[Brady Eidson]

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

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

[Brady Eidson]

Created attachment 316622 [details]
Patch

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

[Brady Eidson]

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

[Brady Eidson]

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

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

[Brady Eidson]

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

[Brady Eidson]

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

[Brady Eidson]

Created attachment 316647 [details]
Patch

[Brady Eidson]

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.

[WebKit Commit Bot]

Comment on attachment 316647 [details]
Patch

Clearing flags on attachment: 316647

Committed r220011: <http://trac.webkit.org/changeset/220011>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.