| Summary: | Release assert in updateLayout() via AXObjectCache::childrenChanged | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
| Component: | Layout and Rendering | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, bfulgham, cdumez, cfleizach, commit-queue, dbates, dmazzoni, esprehn+autocc, ews-watchlist, jcraig, jdiggs, kangil.han, koivisto, rniwa, samuel_white, simon.fraser, zalan | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 332631 [details]
Disables the assertion
Created attachment 332633 [details]
Fixed builds
(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, (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. (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 (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... (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 (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. 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 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
Comment on attachment 332633 [details]
Fixed builds
r=me
> 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.
Comment on attachment 332633 [details]
Fixed builds
I don't think the test failure in service worker is related to my change.
Comment on attachment 332633 [details] Fixed builds Clearing flags on attachment: 332633 Committed r227858: <https://trac.webkit.org/changeset/227858> All reviewed patches have been landed. Closing bug. |
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*)