WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(5.49 KB, patch)
2017-07-27 23:07 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(17.06 KB, patch)
2017-07-28 10:52 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2017-07-27 08:54:32 PDT
<
rdar://problem/33568276
>
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
Created
attachment 316622
[details]
Patch
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
Created
attachment 316647
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug