Summary: | DocumentLoader should always notify the client if there are pending icon loads when the load is stopped | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||
Component: | Page Loading | Assignee: | Brady Eidson <beidson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, beidson, buildbot, cdumez, commit-queue, dbates, japhet, mcatanzaro | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 173877 | ||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2017-06-27 07:32:52 PDT
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. |