If DocumentLoader::stopLoading() is called before DocumentLoader::didGetLoadDecisionForIcon() or before DocumentLoader::finishedLoadingIcon(), the UI process never receives the notifications. The load icon decision callbacks in WebPageProxy are not released until it's destroyed, keeping them alive as well as anything captured in the lambdas.
Created attachment 313911 [details] Patch I noticed this while working on moving the GTK+ and WPE APIs to use the new icon loading client. The unit test reported memory leaks, because we keep a reference of the web view in the icon loading decision callback, and the lambda is not destroyed as expected when a new load happens. So, this will be covered by GTK+ and WPE unit tests once I finish the migration.
Comment on attachment 313911 [details] Patch Nice fix! It looks right to me, but please wait until tomorrow to commit in case someone else has review feedback.
(In reply to Michael Catanzaro from comment #2) > Comment on attachment 313911 [details] > Patch > > Nice fix! > > It looks right to me, but please wait until tomorrow to commit in case > someone else has review feedback. Sure, I'll wait for Brady's review.
Comment on attachment 313911 [details] Patch Great find, and this fixes it, but: 1 - I think we can structure it better 2 - I think it needs an explicit API test with the fix.
I'm taking a look at this right now.
Created attachment 314037 [details] Proposed patch while working on test This is the patch I'm working with as I develop a test. It's very close to yours, clearly. Two changes, one stylistic and one functional: Stylistic - Factor out the notifyFinishedLoadingIcon method for the 3 places that use it Functional - In didGetLoadDecisionForIcon it's important to *always* do the take. We can't early return on !decision or !m_frame without first doing the take.
API test to come a bit later today
(In reply to Brady Eidson from comment #6) > Created attachment 314037 [details] > Proposed patch while working on test > > This is the patch I'm working with as I develop a test. > > It's very close to yours, clearly. Two changes, one stylistic and one > functional: > > Stylistic - Factor out the notifyFinishedLoadingIcon method for the 3 places > that use it > Functional - In didGetLoadDecisionForIcon it's important to *always* do the > take. We can't early return on !decision or !m_frame without first doing the > take. Good point, sounds good to me, thanks!
Created attachment 314047 [details] Patch
Created attachment 314051 [details] Patch
Comment on attachment 314051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314051&action=review r=me > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IconLoadingDelegate.mm:224 > + RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]); Apparently auto is the cool new thing to do in places like this.
(In reply to Alex Christensen from comment #11) > Comment on attachment 314051 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314051&action=review > > r=me > > > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IconLoadingDelegate.mm:224 > > + RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]); > > Apparently auto is the cool new thing to do in places like this. Yah, I just copied from the other test in the file, I'm just going to leave it internally consistent :)
Comment on attachment 314051 [details] Patch Clearing flags on attachment: 314051 Committed r218896: <http://trac.webkit.org/changeset/218896>
All reviewed patches have been landed. Closing bug.