Bug 143517

Summary: WebCore IconDatabase retain counts can get unbalanced when WebContent process terminates
Product: WebKit Reporter: Gordon Sheridan <gordon_sheridan>
Component: WebKit Misc.Assignee: Gordon Sheridan <gordon_sheridan>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, beidson, commit-queue, jberlin
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.10   
Attachments:
Description Flags
Patch to track IconDatabase retain counts for WebContent processes.
beidson: review-
Updated patch to track IconDatabase retain counts for WebContent processes. none

Description Gordon Sheridan 2015-04-08 00:43:28 PDT
The WebCore IconDatabase lives in the UI process. The WebContent process sends RetainIconForPageURL/ReleaseIconForPageURL messages to the UI process via WebIconDatabaseProxy. When a page is closed, its associated HistoryItems are released, which in turn queue ReleaseIconForPageURL messages for their URLs. However, the process terminates before the messages are actually sent to the UI process, so the WebCore IconDatabase is not able to balance the retain counts.
Comment 1 Gordon Sheridan 2015-04-08 00:51:02 PDT
Brady Eidson suggested WebProcessProxy the retain/releases in the UI process, and issue any remaining releases necessary when the associated process terminates. I expect to post a patch implementing this suggestion shortly.
Comment 2 Gordon Sheridan 2015-04-08 02:55:52 PDT
Created attachment 250344 [details]
Patch to track IconDatabase retain counts for WebContent processes.
Comment 3 Gordon Sheridan 2015-04-08 03:01:25 PDT
This is associated with rdar://problem/17388461.
Comment 4 Brady Eidson 2015-04-08 10:13:51 PDT
Comment on attachment 250344 [details]
Patch to track IconDatabase retain counts for WebContent processes.

View in context: https://bugs.webkit.org/attachment.cgi?id=250344&action=review

Looks great. Just a couple of tweaks needed.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:443
> +    if (!iconDatabase || pageURL.isNull())

Make this check isEmpty() instead of isNull()

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:457
> +    if (!iconDatabase || pageURL.isNull())

Make this check isEmpty() instead of isNull()

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:465
> +    --result->value;

If the new value is 0, we should remove the entry from the map.

Otherwise, a long lived WebProcess used for lots of navigations might build up a lot of leaked cruft inside this map.
Comment 5 Gordon Sheridan 2015-04-09 01:48:01 PDT
Created attachment 250427 [details]
Updated patch to track IconDatabase retain counts for WebContent processes.

Updated patch to incorporate Brady's comments.
Comment 6 WebKit Commit Bot 2015-04-09 15:38:30 PDT
Comment on attachment 250427 [details]
Updated patch to track IconDatabase retain counts for WebContent processes.

Clearing flags on attachment: 250427

Committed r182610: <http://trac.webkit.org/changeset/182610>
Comment 7 WebKit Commit Bot 2015-04-09 15:38:35 PDT
All reviewed patches have been landed.  Closing bug.