Bug 53100 - WK2 leaks when a page is closed
Summary: WK2 leaks when a page is closed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-01-25 10:45 PST by chris fleizach
Modified: 2011-01-25 17:55 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.27 KB, patch)
2011-01-25 11:00 PST, chris fleizach
sam: review-
Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2011-01-25 11:41 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (6.77 KB, patch)
2011-01-25 13:12 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (7.08 KB, patch)
2011-01-25 14:39 PST, chris fleizach
cfleizach: review-
cfleizach: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2011-01-25 10:45:32 PST
It looks like the remote accessibility element holds strong references to the window object. 
To remedy, WKView should release those references when the page closes.
Comment 1 mitz 2011-01-25 10:48:16 PST
<rdar://problem/8868239>
Comment 2 chris fleizach 2011-01-25 11:00:39 PST
Created attachment 80084 [details]
Patch
Comment 3 Build Bot 2011-01-25 11:31:07 PST
Attachment 80084 [details] did not build on win:
Build output: http://queues.webkit.org/results/7582371
Comment 4 chris fleizach 2011-01-25 11:41:39 PST
Created attachment 80088 [details]
Patch
Comment 5 Early Warning System Bot 2011-01-25 11:43:28 PST
Attachment 80084 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7516293
Comment 6 Sam Weinig 2011-01-25 11:50:35 PST
Comment on attachment 80084 [details]
Patch

This needs to stub out pageClosed in the other PageClients (win/WebView.h, API/qt/qwkpage_p.h). We may also want to do this when the process crashes.
Comment 7 Early Warning System Bot 2011-01-25 12:00:47 PST
Attachment 80088 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7614333
Comment 8 Darin Adler 2011-01-25 12:01:41 PST
Comment on attachment 80088 [details]
Patch

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

Please don’t break the Qt build, otherwise looks good.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1405
> +    WKAXInitializeRemoteElementWithWindow(_data->_remoteAccessibilityChild.get(), nil);

The use of initialize in the name of this function makes it surprising that you can use it to clear the window. Perhaps it should be called WKAXSetWindowForRemoteElement?

> Source/WebKit2/UIProcess/win/WebView.cpp:631
> +void WebView::pageClosed()
> +{
> +}

I think you’ll need this for Qt too, in UIProcess/API/qt/qwkpage.cpp.
Comment 9 chris fleizach 2011-01-25 12:44:56 PST
> The use of initialize in the name of this function makes it surprising that you can use it to clear the window. Perhaps it should be called WKAXSetWindowForRemoteElement?
> 

file https://bugs.webkit.org/show_bug.cgi?id=53118 to handle the rename
Comment 10 Anders Carlsson 2011-01-25 12:52:21 PST
(In reply to comment #9)
> > The use of initialize in the name of this function makes it surprising that you can use it to clear the window. Perhaps it should be called WKAXSetWindowForRemoteElement?
> > 
> 
> file https://bugs.webkit.org/show_bug.cgi?id=53118 to handle the rename

Instead of doing this in close, could we do it in viewDidMoveToWindow when the new window is nil?

That way you wouldn't always leak the WKView unless you explicitly call close.
Comment 11 chris fleizach 2011-01-25 12:56:30 PST
(In reply to comment #10)
> (In reply to comment #9)
> > > The use of initialize in the name of this function makes it surprising that you can use it to clear the window. Perhaps it should be called WKAXSetWindowForRemoteElement?
> > > 
> > 
> > file https://bugs.webkit.org/show_bug.cgi?id=53118 to handle the rename
> 
> Instead of doing this in close, could we do it in viewDidMoveToWindow when the new window is nil?
> 
> That way you wouldn't always leak the WKView unless you explicitly call close.

It looks like viewDidMoveToWindow is not called when the window is closed
Comment 12 chris fleizach 2011-01-25 13:12:25 PST
Created attachment 80110 [details]
Patch
Comment 13 Darin Adler 2011-01-25 14:03:29 PST
Comment on attachment 80110 [details]
Patch

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

This patch failed to apply because of a merge conflict, so we are not getting EWS results for it. You probably should merge and post a new patch so we can see if it breaks the build or not.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1412
> +    [self _setRemoteAccessibilityWindow:nil];

We may want to do this in viewDidMoveToWindow as well, as Anders suggested, even though we can’t do it only there.
Comment 14 chris fleizach 2011-01-25 14:39:57 PST
Created attachment 80126 [details]
Patch
Comment 15 chris fleizach 2011-01-25 14:42:01 PST
adding patch to see if it builds on qt
Comment 16 chris fleizach 2011-01-25 17:54:26 PST
http://trac.webkit.org/changeset/76657
Comment 17 chris fleizach 2011-01-25 17:55:02 PST
Comment on attachment 80126 [details]
Patch

already reviewed by darin