ElementInnerText: guard PlainText() against unavailable OffsetMapping [chromium/src : main]

0 views

Skip to first unread message

Helmut Januschka (Gerrit)

unread,

12:52 PM (8 hours ago) 12:52 PM

to Helmut Januschka, AyeAye, Morten Stenshorne, Philip Jägenstedt, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Attention needed from Morten Stenshorne

Helmut Januschka added 3 comments

File third_party/blink/renderer/core/frame/frame_content_as_text.cc

Line 29, Patchset 9: if (frame->View()->NeedsLayout() || document->NeedsLayoutTreeUpdate()) {

Morten Stenshorne . unresolved

FrameContentAsText() is on the callstack in less than 7% of the crashes, so this isn't going to fix the bug.
That said, whether it's correct or not to update style and layout here, I'm not convinced. It seems to be called more or less directly from non-Blink code, which in a way suggests that we need to take care of such things, but, then again, didn't someone add these DCHECKs for a reason? And why is ChromeRenderFrameObserver::DidMeaningfulLayout() on the call stack? If we did layout, why do we need layout?
I don't think this is the right fix.
If it is, though, we should call LocalFrameView::UpdateLifecycleToLayoutClean() instead, and without checking for dirtyness.

Helmut Januschka

reverted all frame_content_as_text.cc changes completely, DCHECKs restored,innerText() restored.

The fix/try is now in element_inner_text.cc, guarding the PlainText() calls before they hit the null OffsetMapping.
This covers the most of crashes coming through accessibility callers (like GetInnerTextWithoutUpdate() call sites in ax_node_object.cc/ax_object.cc) that run post-layout and can't rewind the lifecycle.

To your question about DidMeaningfulLayout, its dispatched in WebFrameWidgetImpl::UpdateLifecycle() after GetPage()->UpdateLifecycle() completes.

The 7% FrameContentAsText path goes through innerText() which calls UpdateStyleAndLayoutForNode(), but GetInnerTextWithoutUpdate() itself has the clean-layout DCHECK intentionally commented out (crbug.com/1165850, crbug.com/1166296) acknowledging layout isn't always clean.

let me know if that direction is a feasable one, if not i might need to abandon, as i can't see the forest for the trees anymore.

Line 33, Patchset 9: if (frame->View()->NeedsLayout() || document->NeedsLayoutTreeUpdate()) {

Morten Stenshorne . resolved

Replace with original DCHECKs.

Helmut Januschka

Done

Line 39, Patchset 9: output.Append(document->documentElement()->GetInnerTextWithoutUpdate());

Morten Stenshorne . resolved

Why this?

Helmut Januschka

Done

Open in Gerrit

Related details

Attention is currently required from:

Morten Stenshorne

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages