Bug 293511 – ASAN_TRAP | WebCore::LocalFrameViewLayoutContext::performLayout; WebCore::LocalFrameViewLayoutContext::layout; WebCore::Document::updateLayout

• Status: REOPENED
• Component: New Bugs

[Adan Lopez]

Created attachment 475362 [details]
test_case

Stack Trace
=========

frame #0: WebCore`WebCore::LocalFrameViewLayoutContext::performLayout(bool)+0x1e64
frame #1: WebCore`WebCore::LocalFrameViewLayoutContext::layout(bool)+0x128
frame #2: WebCore`WebCore::Document::updateLayout(WTF::OptionSet<WebCore::LayoutOptions>, WebCore::Element const*)+0xa64
frame #3: WebCore`WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const+0xbc
frame #4: WebCore`WebCore::Document::execCommand(WTF::String const&, bool, std::__1::variant<WTF::String, WTF::RefPtr<WebCore::TrustedHTML, WTF::RawPtrTraits<WebCore::TrustedHTML>, WTF::DefaultRefDerefTraits<WebCore::TrustedHTML>>> const&)+0x364
frame #5: WebCore`WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)+0x478
frame #6: WebCore`WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*)+0xd4
frame #7: `0x000300018040+
frame #8: `0x0003001308a0+
frame #9: `0x000300058100+
frame #10: `0x00030006ccf4+
frame #11: `0x000300008544+
frame #12: JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)+0x6e8
frame #13: JavaScriptCore`JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x158
frame #14: WebCore`WebCore::MicrotaskQueue::performMicrotaskCheckpoint()+0x6e0
frame #15: WebCore`WebCore::EventLoop::run(std::__1::optional<WTF::ApproximateTime>)+0x40c
frame #16: WebCore`WebCore::WindowEventLoop::didReachTimeToRun()+0x10c
frame #17: WebCore`WTF::Detail::CallableWrapper<WebCore::Timer::Timer<WebCore::WindowEventLoop, WebCore::WindowEventLoop>(WebCore::WindowEventLoop&, void (WebCore::WindowEventLoop::*)())::'lambda'(), void>::call()+0x16c
frame #18: WebCore`WebCore::ThreadTimers::sharedTimerFiredInternal()+0x320
frame #19: WebCore`WebCore::timerFired(__CFRunLoopTimer*, void*)+0x7c
frame #20: CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__+0x1c
frame #21: CoreFoundation`__CFRunLoopDoTimer+0x3d0
frame #22: CoreFoundation`__CFRunLoopDoTimers+0x114
frame #23: CoreFoundation`__CFRunLoopRun+0x734
frame #24: CoreFoundation`CFRunLoopRunSpecific+0x22c
frame #25: Foundation`-[NSRunLoop(NSRunLoop) runMode:beforeDate:]+0xd0
frame #26: Foundation`-[NSRunLoop(NSRunLoop) run]+0x3c
frame #27: libxpc.dylib`_xpc_objc_main+0x2b8
frame #28: libxpc.dylib`_xpc_main+0x24
frame #29: libxpc.dylib`xpc_main+0x3c
frame #30: WebKit`WebKit::XPCServiceMain(int, char const**)+0xa0
frame #31: `0x00018dd47afc+

[Adan Lopez]

rdar://147938727

[Adan Lopez]

Pull request: https://github.com/WebKit/WebKit/pull/45975

[alan]

Created attachment 475524 [details]
Patch

[Devin Rousso]

Comment on attachment 475524 [details]
Patch

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

> Source/WebCore/inspector/agents/page/PageTimelineAgent.cpp:199
> +    TimelineRecordFactory::appendLayoutRoot(entry->data.get(), layoutArea);

this is technically a difference in behavior as will get called regardless of how many values are returned by `RenderObject::absoluteQuads`

id suggest passing in `Vector<FloatQuad>` to this function so that 1) we can preserve as much existing behavior as possible and 2) we can avoid doing work when Web Inspector is not open

i suppose that the `ASSERT` implies it should never happen, but i'd rather keep it there (unless you can prove it will never be an issue, in which case i think what you have overall is fine)

> Source/WebCore/page/LocalFrameViewLayoutContext.cpp:247
> +    auto layoutArea = FloatQuad { };

continuing from my comment and reasoning above, id replace this and the modified lines below with the following
```cpp
Vector<FloatQuad> layoutAreas;
{
    ...

    layoutRoot->absoluteQuads(layoutAreas);

    ...
}
InspectorInstrumentation::didLayout(frame, layoutAreas);
```

[alan]

(In reply to Devin Rousso from comment #4)
> Comment on attachment 475524 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=475524&action=review
> 
> > Source/WebCore/inspector/agents/page/PageTimelineAgent.cpp:199
> > +    TimelineRecordFactory::appendLayoutRoot(entry->data.get(), layoutArea);
> 
> this is technically a difference in behavior as will get called regardless
> of how many values are returned by `RenderObject::absoluteQuads`
> 
> id suggest passing in `Vector<FloatQuad>` to this function so that 1) we can
> preserve as much existing behavior as possible and 2) we can avoid doing
> work when Web Inspector is not open
> 
> i suppose that the `ASSERT` implies it should never happen, but i'd rather
> keep it there (unless you can prove it will never be an issue, in which case
> i think what you have overall is fine)
> 
> > Source/WebCore/page/LocalFrameViewLayoutContext.cpp:247
> > +    auto layoutArea = FloatQuad { };
> 
> continuing from my comment and reasoning above, id replace this and the
> modified lines below with the following
> ```cpp
> Vector<FloatQuad> layoutAreas;
> {
>     ...
> 
>     layoutRoot->absoluteQuads(layoutAreas);
> 
>     ...
> }
> InspectorInstrumentation::didLayout(frame, layoutAreas);
> ```

Honestly I don't even understand why quads are used in here as the layout area is never transformed, painting yes, layout area is never. also 99% of the time this is called on RenderView which just returns the size of the associated layer. Do you have some insights here?

[Devin Rousso]

Comment on attachment 475524 [details]
Patch

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

>>> Source/WebCore/inspector/agents/page/PageTimelineAgent.cpp:199
>>> +    TimelineRecordFactory::appendLayoutRoot(entry->data.get(), layoutArea);
>> 
>> this is technically a difference in behavior as will get called regardless of how many values are returned by `RenderObject::absoluteQuads`
>> 
>> id suggest passing in `Vector<FloatQuad>` to this function so that 1) we can preserve as much existing behavior as possible and 2) we can avoid doing work when Web Inspector is not open
>> 
>> i suppose that the `ASSERT` implies it should never happen, but i'd rather keep it there (unless you can prove it will never be an issue, in which case i think what you have overall is fine)
> 
> Honestly I don't even understand why quads are used in here as the layout area is never transformed, painting yes, layout area is never. also 99% of the time this is called on RenderView which just returns the size of the associated layer. Do you have some insights here?

i believe this is used to populate the Size, Width, and Height columns of Layout records in the Layout & Rendering timeline in the Timelines Tab

the idea is to give developers a way of understanding what part(s) of the page did layout (i.e. the area on the screen), as well as how long that layout took (though that info is derived elsewhere)

this part of the Timelines Tab is quite old so it's possible that some aspects of the motivation (e.g. example scenarios) have been lost to time :(

[alan]

> i believe this is used to populate the Size, Width, and Height columns of
> Layout records in the Layout & Rendering timeline in the Timelines Tab
oh ok. It sounds like it is supposed to be some kind of enclosing rect. I just find it interesting that we collect these quads just to ignore some of them (using only the first one in the list). Thanks for explaining it.

[alan]

Created attachment 475537 [details]
Patch

[alan]

Devin, do you mind taking another look at it? Thank you.

[Devin Rousso]

Comment on attachment 475537 [details]
Patch

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

oh!  my apologies.  i thought you were using this as a WIP before putting the actual changes in a GitHub PR

this looks great :)

> Source/WebCore/page/LocalFrameViewLayoutContext.cpp:295
> +    InspectorInstrumentation::didLayout(frame, layoutAreas);

NIT: should we `WTFMove(layoutAreas)` instead since it's not used anywhere else?

[alan]

Thank you for the review. I talked to Chris about the WTFMove and the current thinking is that we don't use move unless transferring ownership.

[EWS]

Committed 296484@main (ad4a1079d2dc): <https://commits.webkit.org/296484@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 475537 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 295372