Bug 173874

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 LoadingAssignee: 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 Flags
Patch
none
Proposed patch while working on test
none
Patch
none
Patch none

Description Carlos Garcia Campos 2017-06-27 07:32:52 PDT
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.
Comment 1 Carlos Garcia Campos 2017-06-27 07:40:08 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 2 Michael Catanzaro 2017-06-27 07:55:58 PDT
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.
Comment 3 Carlos Garcia Campos 2017-06-27 08:20:22 PDT
(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 4 Brady Eidson 2017-06-28 10:00:55 PDT
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.
Comment 5 Brady Eidson 2017-06-28 10:01:08 PDT
I'm taking a look at this right now.
Comment 6 Brady Eidson 2017-06-28 10:32:11 PDT
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.
Comment 7 Brady Eidson 2017-06-28 10:32:35 PDT
API test to come a bit later today
Comment 8 Carlos Garcia Campos 2017-06-28 11:00:45 PDT
(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!
Comment 9 Brady Eidson 2017-06-28 12:11:40 PDT
Created attachment 314047 [details]
Patch
Comment 10 Brady Eidson 2017-06-28 12:24:33 PDT
Created attachment 314051 [details]
Patch
Comment 11 Alex Christensen 2017-06-28 12:34:12 PDT
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.
Comment 12 Brady Eidson 2017-06-28 15:12:10 PDT
(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 13 WebKit Commit Bot 2017-06-28 15:40:13 PDT
Comment on attachment 314051 [details]
Patch

Clearing flags on attachment: 314051

Committed r218896: <http://trac.webkit.org/changeset/218896>
Comment 14 WebKit Commit Bot 2017-06-28 15:40:15 PDT
All reviewed patches have been landed.  Closing bug.