Bug 173874 – DocumentLoader should always notify the client if there are pending icon loads when the load is stopped

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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.

[Brady Eidson]

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.

[Brady Eidson]

I'm taking a look at this right now.

[Brady Eidson]

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.

[Brady Eidson]

API test to come a bit later today

[Carlos Garcia Campos]

(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!

[Brady Eidson]

Created attachment 314047 [details]
Patch

[Brady Eidson]

Created attachment 314051 [details]
Patch

[Alex Christensen]

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.

[Brady Eidson]

(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 :)

[WebKit Commit Bot]

Comment on attachment 314051 [details]
Patch

Clearing flags on attachment: 314051

Committed r218896: <http://trac.webkit.org/changeset/218896>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.