RESOLVED FIXED 174895
WKURLSchemeTaskImpl instances are being abandoned (except if the task is stopped)
https://bugs.webkit.org/show_bug.cgi?id=174895
Summary WKURLSchemeTaskImpl instances are being abandoned (except if the task is stop...
mitz
Reported 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).
Attachments
Test project (13.59 KB, application/zip)
2017-07-27 08:53 PDT, mitz
no flags
Patch (5.49 KB, patch)
2017-07-27 23:07 PDT, Brady Eidson
no flags
Patch (17.06 KB, patch)
2017-07-28 10:52 PDT, Brady Eidson
no flags
Radar WebKit Bug Importer
Comment 1 2017-07-27 08:54:32 PDT
Brady Eidson
Comment 2 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.
Brady Eidson
Comment 3 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)
mitz
Comment 4 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.
Brady Eidson
Comment 5 2017-07-27 23:07:47 PDT
mitz
Comment 6 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
Brady Eidson
Comment 7 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!
Brady Eidson
Comment 8 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.
mitz
Comment 9 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.
Brady Eidson
Comment 10 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.
Brady Eidson
Comment 11 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.
Brady Eidson
Comment 12 2017-07-28 10:52:14 PDT
Brady Eidson
Comment 13 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.
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2017-07-28 13:03:48 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.