[WK2] AlternativeTextClient leaks when the page is destroyed

[Andreas Kling]

There is nobody deleting the WebKit::WebAlternativeTextClient AFAICT. Opening and closing a couple of pages, and then running "leaks" will show you that a bunch of WebAlternativeTextClient objects have leaked.

[Jia Pu]

Will take a look.

[Jia Pu]

Created attachment 137942 [details]
Fix memory leak.

[Eric Seidel (no email)]

I thought we had a PageObserver pattern these days?

[Eric Seidel (no email)]

I guess only FrameDestructionObserver.

[Ryosuke Niwa]

Comment on attachment 137942 [details]
Fix memory leak.

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

> Source/WebKit/mac/WebCoreSupport/WebAlternativeTextClient.mm:45
> +void WebAlternativeTextClient::pageDestroyed()
> +{
> +    delete this;
> +}

Really? We need to manually call delete on this? Can't we use OwnPtr instead?

[Jia Pu]

(In reply to comment #5)
> (From update of attachment 137942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137942&action=review
> 
> > Source/WebKit/mac/WebCoreSupport/WebAlternativeTextClient.mm:45
> > +void WebAlternativeTextClient::pageDestroyed()
> > +{
> > +    delete this;
> > +}
> 
> Really? We need to manually call delete on this? Can't we use OwnPtr instead?

I am just following this pattern in various other clients. I can certainly just release it on WebCore side.

[Tim Horton]

Comment on attachment 137942 [details]
Fix memory leak.

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

> Source/WebKit2/ChangeLog:8
> +        Fixing memory leak.

The changelog entry could definitely be slightly more informative re: what situation we were leaking in and how it's fixed; as it stands, it's impossible to know much about what's going on without looking at the change itself.

[Jia Pu]

Created attachment 138421 [details]
Use OwnPtr.

[Darin Adler]

The other clients aren’t owned like this. Why is the pattern different for this one?

[Darin Adler]

(In reply to comment #5)
> We need to manually call delete on this? Can't we use OwnPtr instead?

We shouldn’t because the ownership is not something Page enforces. It’s something that can vary per-port. If we were going to use OwnPtr we’d want to use it in the clients structure itself and remove the flexibility of having the clients not all be separate objects, flexibility we have today.

[Darin Adler]

Another way to put it is that if we are going to use OwnPtr, then the adoptPtr needs to be right next to the new, not inside the Page constructor’s implementation.

[Jia Pu]

Would the first patch look OK then?

[Jon Lee]

*** Bug 84984 has been marked as a duplicate of this bug. ***

[Jon Lee]

<rdar://problem/11328431>

[Jon Lee]

I had filed a similar bug and posted a patch exactly like the _first_ one that Jia had posted. The pattern, while not ideal, is established in other clients. So given that my patch is exactly what Jia did, I would unofficially r+ the first patch.

[Jon Lee]

Talked to Jia offline, reassigning to me.

[Jon Lee]

Created attachment 139071 [details]
Patch

[Enrica Casucci]

Comment on attachment 139071 [details]
Patch

Looks good.

[Jon Lee]

Committed r115369: <http://trac.webkit.org/changeset/115369>