Bug 219905

Summary: ASSERTION FAILED: &layoutState().establishedFormattingState(layoutBox.formattingContextRoot()) == this in WebCore::Layout::FormattingState::boxGeometry
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, sam, simon.fraser, tsavell, webkit-bot-watchers-bugzilla, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
crash log
none
Patch
simon.fraser: review+
Patch none

Description Ryan Haddad 2020-12-15 11:28:56 PST 
fast/layoutformattingcontext/subframe-with-display-none-html.html is a flaky crash on debug macOS bots

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000002524f11ce WTFCrash + 14 (Assertions.cpp:295)
1   com.apple.WebCore             	0x000000022f6b51ab WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x0000000233145c2d WebCore::Layout::FormattingState::boxGeometry(WebCore::Layout::Box const&) + 141 (FormattingState.cpp:54)
3   com.apple.WebCore             	0x0000000233146343 WebCore::Layout::FormattingContext::computeBorderAndPadding(WebCore::Layout::Box const&, WebCore::Layout::HorizontalConstraints const&) + 51 (FormattingContext.cpp:131)
4   com.apple.WebCore             	0x00000002331730bb WebCore::Layout::BlockFormattingContext::layoutInFlowContent(WebCore::Layout::InvalidationState&, WebCore::Layout::FormattingContext::ConstraintsForInFlowContent const&) + 635 (BlockFormattingContext.cpp:115)
5   com.apple.WebCore             	0x00000002331545e7 WebCore::Layout::LayoutContext::layoutFormattingContextSubtree(WebCore::Layout::ContainerBox const&, WebCore::Layout::InvalidationState&) + 327 (LayoutContext.cpp:111)
6   com.apple.WebCore             	0x00000002331542b7 WebCore::Layout::LayoutContext::layoutWithPreparedRootGeometry(WebCore::Layout::InvalidationState&) + 151 (LayoutContext.cpp:87)
7   com.apple.WebCore             	0x000000023315415b WebCore::Layout::LayoutContext::layout(WebCore::LayoutSize const&, WebCore::Layout::InvalidationState&) + 443 (LayoutContext.cpp:78)
8   com.apple.WebCore             	0x000000023354475d WebCore::FrameViewLayoutContext::layoutUsingFormattingContext() + 349 (FrameViewLayoutContext.cpp:75)
9   com.apple.WebCore             	0x0000000233525c9d WebCore::FrameViewLayoutContext::layout() + 2141 (FrameViewLayoutContext.cpp:234)
10  com.apple.WebCore             	0x0000000233544edf WebCore::FrameViewLayoutContext::layoutTimerFired() + 127 (FrameViewLayoutContext.cpp:469)

https://results.webkit.org/?suite=layout-tests&test=fast%2Flayoutformattingcontext%2Fsubframe-with-display-none-html.html
Comment 1 Ryan Haddad 2020-12-15 11:29:51 PST 
This is the assert from FormattingState.cpp:54
>    // Should never need to mutate a display box outside of the formatting context.
>    > ASSERT(&layoutState().establishedFormattingState(layoutBox.formattingContextRoot()) == this);
Comment 2 Ryan Haddad 2020-12-15 11:30:40 PST 
Created attachment 416269 [details]
crash log
Comment 3 Radar WebKit Bug Importer 2020-12-15 11:31:28 PST 
<rdar://problem/72350516>
Comment 4 Ryan Haddad 2020-12-15 11:32:37 PST 
The first instance of this I see appears to be at https://trac.webkit.org/changeset/269627/webkit
Comment 5 Truitt Savell 2021-01-12 15:21:43 PST 
I am able to reproduce this using command 
rwt fast/layoutformattingcontext/subframe-with-display-none-html.html --iterations 2000 -f --exit-after-n-crashes-or-timeout 1
Comment 6 Truitt Savell 2021-01-13 10:17:32 PST 
there is a large regression range due to a lack of builds during this time. I can reproduce this on 269629 but not on 269601
Comment 7 Truitt Savell 2021-01-13 13:39:14 PST 
marked this test as crashing in https://trac.webkit.org/changeset/271454/webkit
Comment 8 Ryan Haddad 2021-03-22 14:12:35 PDT 
I skipped the test in https://trac.webkit.org/changeset/274083/webkit because it kept showing up in the "other crashes" section of test results.
Comment 9 zalan 2021-04-08 19:53:35 PDT 
Created attachment 425577 [details]
Patch
Comment 10 zalan 2021-04-09 20:01:05 PDT 
So this is the very similar to what was already addressed in bug 219878.

In TestController::resetPreferencesToConsistentValues, first we call WKPreferencesResetAllInternalDebugFeatures() which (in batch) turns on/off the debug features. It enables LFC integration while it disables full LFC. Later in this function we take the boolWebPreferenceFeatures() unordered map and call WKPreferencesSetBoolValueForKeyForTesting() on each entry.
WKPreferencesSetBoolValueForKeyForTesting is not a batch update, so it'll (through the WebPageProxy) issue a preferencesDidChange messages on each call.
When we reach the "LayoutFormattingContextEnabled=true" key/value in this unordered map and update the preferences, WebContentProcess may see both the integration (from WKPreferencesResetAllInternalDebugFeatures) and the full LFC (this call) on.
Later when the loop reaches "LayoutFormattingContextIntegrationEnabled=false" and we update the preferences, everything goes back to normal. However if WebContent process issues a layout in the meantime, we hit this assert.

There are a few ways to address it.
1, use batch update for all the test preferences
2, use some kind of ordered data structure to guarantee that "<!-- webkit-test-runner [ LayoutFormattingContextIntegrationEnabled=false LayoutFormattingContextEnabled=true  ] -->" turns the integration feature off first and then enables full LFC.
3, since these 2 preferences are mutually exclusive, turning full LFC on would also disable the LFC integration bit.
Comment 11 zalan 2021-04-12 12:45:42 PDT 
Created attachment 425776 [details]
Patch
Comment 12 EWS 2021-04-12 13:31:31 PDT 
Committed r275837 (236405@main): <https://commits.webkit.org/236405@main>

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