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).
<rdar://problem/33568276>
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.
(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.
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
(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!
(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.
(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.
(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.
Created attachment 316647 [details] Patch
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 on attachment 316647 [details] Patch Clearing flags on attachment: 316647 Committed r220011: <http://trac.webkit.org/changeset/220011>
All reviewed patches have been landed. Closing bug.