Bug 182279 – Release assert in updateLayout() via AXObjectCache::childrenChanged

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

e.g.
      50 WebCore: WebCore::Document::updateLayout() <==
        50 WebCore: WebCore::Document::updateLayout()
          47 WebCore: WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks)
          | 47 WebCore: WebCore::AccessibilityObject::updateBackingStore()
          |   47 WebCore: -[WebAccessibilityObjectWrapper _prepareAccessibilityCall]
          |     47 WebCore: -[UIKitWebAccessibilityObjectWrapper _prepareAccessibilityCall]
          |       47 WebCore: -[WebAccessibilityObjectWrapper attachmentView]
          |         47 WebCore: WebCore::AccessibilityObject::accessibilityIgnoreAttachment() const
          |           47 WebCore: WebCore::AccessibilityObject::accessibilityIsIgnored() const
          |             47 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
          |               47 WebCore: WebCore::AccessibilityObject::defaultObjectInclusion() const
          |                 47 WebCore: WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
          |                   47 WebCore: WebCore::AccessibilityObject::accessibilityIsIgnored() const
          |                     47 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
          |                       47 WebCore: WebCore::AccessibilityObject::defaultObjectInclusion() const
          |                         47 WebCore: WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
          |                           47 WebCore: WebCore::AccessibilityObject::accessibilityIsIgnored() const
          |                             47 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
          |                               47 WebCore: WebCore::AccessibilityObject::defaultObjectInclusion() const
          |                                 47 WebCore: WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
          |                                   47 WebCore: WebCore::AccessibilityObject::accessibilityIsIgnored() const
          |                                     47 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
          |                                       47 WebCore: WebCore::AccessibilityObject::defaultObjectInclusion() const
          |                                         47 WebCore: WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
          |                                           47 WebCore: WebCore::AccessibilityObject::accessibilityIsIgnored() const
          |                                             47 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
          |                                               47 WebCore: WebCore::AccessibilityObject::defaultObjectInclusion() const
          |                                                 47 WebCore: WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
          |                                                   47 WebCore: WebCore::AccessibilityObject::accessibilityIsIgnored() const
          |                                                     47 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
          |                                                       47 WebCore: WebCore::AccessibilityObject::defaultObjectInclusion() const
          |                                                         47 WebCore: WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
          |                                                           47 WebCore: WebCore::AccessibilityObject::accessibilityIsIgnored() const
          |                                                             47 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
          |                                                               35 WebCore: WebCore::AXObjectCache::handleLiveRegionCreated(WebCore::Node*)
          |                                                               | 35 WebCore: WebCore::AXObjectCache::childrenChanged(WebCore::RenderObject*, WebCore::RenderObject*)
          |                                                               |   35 WebCore: WebCore::RenderElement::insertChildInternal(std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)
          |                                                               |     35 WebCore: WebCore::RenderElement::addChild(std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>, WebCore::RenderObject*)

[Ryosuke Niwa]

<rdar://problem/36994456>

[Ryosuke Niwa]

Created attachment 332631 [details]
Disables the assertion

[Ryosuke Niwa]

Created attachment 332633 [details]
Fixed builds

[chris fleizach]

(In reply to Ryosuke Niwa from comment #1)
> <rdar://problem/36994456>

What line do we go from AXObjectCache to RenderElement::insert

| 35 WebCore: WebCore::AXObjectCache::childrenChanged(WebCore::RenderObject*, WebCore::RenderObject*)
          |                                                               |   35 WebCore: WebCore::RenderElement::insertChildInternal(std::__1::unique_ptr<WebCore::RenderObject,

[Ryosuke Niwa]

(In reply to chris fleizach from comment #4)
> (In reply to Ryosuke Niwa from comment #1)
> > <rdar://problem/36994456>
> 
> What line do we go from AXObjectCache to RenderElement::insert
> 
> | 35 WebCore:
> WebCore::AXObjectCache::childrenChanged(WebCore::RenderObject*,
> WebCore::RenderObject*)
>           |                                                               | 
> 35 WebCore:
> WebCore::RenderElement::insertChildInternal(std::__1::unique_ptr<WebCore::
> RenderObject,

Maybe you're reading it backwards? RenderElement::insertChildInternal is calling AXObjectCache::childrenChanged here.

[chris fleizach]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to chris fleizach from comment #4)
> > (In reply to Ryosuke Niwa from comment #1)
> > > <rdar://problem/36994456>
> > 
> > What line do we go from AXObjectCache to RenderElement::insert
> > 
> > | 35 WebCore:
> > WebCore::AXObjectCache::childrenChanged(WebCore::RenderObject*,
> > WebCore::RenderObject*)
> >           |                                                               | 
> > 35 WebCore:
> > WebCore::RenderElement::insertChildInternal(std::__1::unique_ptr<WebCore::
> > RenderObject,
> 
> Maybe you're reading it backwards? RenderElement::insertChildInternal is
> calling AXObjectCache::childrenChanged here.

Ah I see. We should call childrenChanged after a one shot timer from insertChildInternal

[Ryosuke Niwa]

(In reply to chris fleizach from comment #6)
>
> Ah I see. We should call childrenChanged after a one shot timer from
> insertChildInternal

Post layout/style update task is fine too but we need to queue them up since there could be hundreds of render objects getting updated.

The accessibility tree seems to have a notion of updating a subtree as well, so perhaps that's more fitting when the entire document's style just got resolved instead of keep telling the accessibility tree what RenderObject got inserted, removed, etc...

[chris fleizach]

(In reply to Ryosuke Niwa from comment #7)
> (In reply to chris fleizach from comment #6)
> >
> > Ah I see. We should call childrenChanged after a one shot timer from
> > insertChildInternal
> 
> Post layout/style update task is fine too but we need to queue them up since
> there could be hundreds of render objects getting updated.

Yes absolutely

> 
> The accessibility tree seems to have a notion of updating a subtree as well,
> so perhaps that's more fitting when the entire document's style just got
> resolved instead of keep telling the accessibility tree what RenderObject
> got inserted, removed, etc...

We try to avoid re-updating the entire tree because it could cause VoiceOver to lose focus if the element it was focused on was tossed. Also for perf reasons, we try to limit the scope of the elements we want changed

[Ryosuke Niwa]

(In reply to chris fleizach from comment #8)
>
> > 
> > The accessibility tree seems to have a notion of updating a subtree as well,
> > so perhaps that's more fitting when the entire document's style just got
> > resolved instead of keep telling the accessibility tree what RenderObject
> > got inserted, removed, etc...
> 
> We try to avoid re-updating the entire tree because it could cause VoiceOver
> to lose focus if the element it was focused on was tossed. Also for perf
> reasons, we try to limit the scope of the elements we want changed

I understand that but it seems a bit silly if we haven't constructed the render tree yet. In this particular test case, the entire subtree below div never had a RenderObject. But we end up notifying the accessibility tree each time a new RenderObject is created for a node under div.

Instead, we should be able to batch up changes and just let the accessibility code know that we have to re-construct the accessibility tree underneath a div.

We should brainstorm about what would be the best way to coordinate between style/layout updates & the accessibility tree.

[EWS Watchlist]

Comment on attachment 332633 [details]
Fixed builds

Attachment 332633 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6258689

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/navigation-redirect.https.html

[EWS Watchlist]

Created attachment 332638 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Antti Koivisto]

Comment on attachment 332633 [details]
Fixed builds

r=me

[zalan]

> We should brainstorm about what would be the best way to coordinate between
> style/layout updates & the accessibility tree.
I've got a radar on this and it's mostly done except a few notifications like insertChildInternal/takeChild.

[Ryosuke Niwa]

Comment on attachment 332633 [details]
Fixed builds

I don't think the test failure in service worker is related to my change.

[WebKit Commit Bot]

Comment on attachment 332633 [details]
Fixed builds

Clearing flags on attachment: 332633

Committed r227858: <https://trac.webkit.org/changeset/227858>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.