Bug 68636 - Accessibility: AXObjectCache is cleared unnecessarily when non-top Document is detached
Summary: Accessibility: AXObjectCache is cleared unnecessarily when non-top Document i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-22 10:43 PDT by Dominic Mazzoni
Modified: 2011-09-27 10:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.75 KB, patch)
2011-09-27 00:39 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (8.06 KB, patch)
2011-09-27 09:09 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2011-09-22 10:43:34 PDT
Only the top document has its own AXObjectCache, so Document::detach doesn't need to call clearAXObjectCache when the document being detached is not the top. This is needlessly causing the whole accessibility tree to be regenerated every time an iframe on a page reloads.
Comment 1 Dominic Mazzoni 2011-09-27 00:39:44 PDT
Created attachment 108801 [details]
Patch
Comment 2 chris fleizach 2011-09-27 08:53:06 PDT
Comment on attachment 108801 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1830
> +

this has the ability to leak objects that came from an iframe until the whole document is cleared, does it not?

> LayoutTests/accessibility/deleting-iframe-destroys-axcache.html:53
> +            document.getElementById("console").innerText += "\nBefore:\n";

wrong indentation.

> LayoutTests/accessibility/deleting-iframe-destroys-axcache.html:56
> +            // Remove the iframe.

wrong indentation.

> LayoutTests/accessibility/deleting-iframe-destroys-axcache.html:64
> +            document.getElementById("console").innerText += "\nAfter:\n";

wrong indentation.

> LayoutTests/accessibility/deleting-iframe-destroys-axcache.html:66
> +            document.getElementById("console").innerText += "\n";

wrong indentation.

> LayoutTests/accessibility/deleting-iframe-destroys-axcache.html:69
> +            // are now invalid - they've been deleted.

wrong indentation.

> LayoutTests/accessibility/deleting-iframe-destroys-axcache.html:74
> +            // Make sure that the other nodes are unchanged.

wrong indentation.
Comment 3 Dominic Mazzoni 2011-09-27 09:01:51 PDT
(In reply to comment #2)
> (From update of attachment 108801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108801&action=review
> 
> > Source/WebCore/dom/Document.cpp:1830
> > +
> 
> this has the ability to leak objects that came from an iframe until the whole document is cleared, does it not?

No, in the case where the iframe is actually deleted like this, I'm positive that AXObjectCache::remove is actually called on all of the accessibility objects within the iframe's tree. The objects are deleted when their reference count hits zero.

The test already confirms this by checking that the objects have lost their correct role, but I added my own debug logging to confirm it.

If there is any scenario where an iframe is detached but not either deleted or reattached somewhere else in the same page, then we could have a problem - but I can't think of any such circumstance.
Comment 4 Dominic Mazzoni 2011-09-27 09:09:09 PDT
Created attachment 108856 [details]
Patch
Comment 5 chris fleizach 2011-09-27 09:11:19 PDT
Comment on attachment 108856 [details]
Patch

looks good. r=me
Comment 6 WebKit Review Bot 2011-09-27 10:28:52 PDT
Comment on attachment 108856 [details]
Patch

Clearing flags on attachment: 108856

Committed r96127: <http://trac.webkit.org/changeset/96127>
Comment 7 WebKit Review Bot 2011-09-27 10:28:56 PDT
All reviewed patches have been landed.  Closing bug.