Bug 55301 – [REGRESSION r79784?] Can't open Network panel in the inspector (assertion failed in layout)

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Pavel Feldman]

#0	0x1015d9687 in WebCore::Length::setValue at Length.h:91
#1	0x1019104ab in WebCore::FixedTableLayout::calcWidthArray at FixedTableLayout.cpp:121
#2	0x10191095f in WebCore::FixedTableLayout::computePreferredLogicalWidths at FixedTableLayout.cpp:204
#3	0x1020794d3 in WebCore::RenderTable::computePreferredLogicalWidths at RenderTable.cpp:574
#4	0x101f8c035 in WebCore::RenderBox::minPreferredLogicalWidth at RenderBox.cpp:659
#5	0x10207b2dc in WebCore::RenderTable::computeLogicalWidth at RenderTable.cpp:219
#6	0x10207a5f4 in WebCore::RenderTable::layout at RenderTable.cpp:278
#7	0x101f5ed92 in WebCore::RenderBlock::layoutBlockChild at RenderBlock.cpp:1958
#8	0x101f6093f in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1896
#9	0x101f60fb4 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1224
#10	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#11	0x101f83337 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:520
#12	0x101f5df34 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2137
#13	0x101f6128f in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1255
#14	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#15	0x101f83337 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:520
#16	0x101f5df34 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2137
#17	0x101f6128f in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1255
#18	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#19	0x101f83337 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:520
#20	0x101f5df34 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2137
#21	0x101f6128f in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1255
#22	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#23	0x101f83337 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:520
#24	0x101f5df34 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2137
#25	0x101f6128f in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1255
#26	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#27	0x101f83337 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:520
#28	0x101f5df34 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2137
#29	0x101f5fe50 in WebCore::RenderBlock::layoutOnlyPositionedObjects at RenderBlock.cpp:2087
#30	0x101f60a4e in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1135
#31	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#32	0x101f83337 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:520
#33	0x101f5df34 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2137
#34	0x101f5fe50 in WebCore::RenderBlock::layoutOnlyPositionedObjects at RenderBlock.cpp:2087
#35	0x101f60a4e in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1135
#36	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#37	0x101f83337 in WebCore::RenderObject::layoutIfNeeded at RenderObject.h:520
#38	0x101f5df34 in WebCore::RenderBlock::layoutPositionedObjects at RenderBlock.cpp:2137
#39	0x101f5fe50 in WebCore::RenderBlock::layoutOnlyPositionedObjects at RenderBlock.cpp:2087
#40	0x101f60a4e in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:1135
#41	0x101f5fc6e in WebCore::RenderBlock::layout at RenderBlock.cpp:1120
#42	0x1020b6fc3 in WebCore::RenderView::layout at RenderView.cpp:130
#43	0x101966cfc in WebCore::FrameView::layout at FrameView.cpp:906
#44	0x1017a5ef9 in WebCore::Document::updateLayout at Document.cpp:1592
#45	0x1017a85b8 in WebCore::Document::updateLayoutIgnorePendingStylesheets at Document.cpp:1623
#46	0x1018cd774 in WebCore::Element::focus at Element.cpp:1498
#47	0x101c3e412 in WebCore::jsElementPrototypeFunctionFocus at JSElement.cpp:1761
#48	0x279468c001b8 in ??
#49	0x1007e21f7 in JSC::JITCode::execute at JITCode.h:77
#50	0x1007dcc56 in JSC::Interpreter::executeCall at Interpreter.cpp:851
#51	0x10079632d in JSC::call at CallData.cpp:38
#52	0x100847b89 in JSC::JSObject::put at JSObject.cpp:149
#53	0x100815fbd in JSC::JSValue::put at JSObject.h:780
#54	0x10080f0ab in cti_op_put_by_id at JITStubs.cpp:1351
#55	0x100803dcd in WTF::doubleHash at HashTable.h:447
#56	0x1007e21f7 in JSC::JITCode::execute at JITCode.h:77
#57	0x1007dcc56 in JSC::Interpreter::executeCall at Interpreter.cpp:851
#58	0x10079632d in JSC::call at CallData.cpp:38
#59	0x100847b89 in JSC::JSObject::put at JSObject.cpp:149
#60	0x100815fbd in JSC::JSValue::put at JSObject.h:780
#61	0x10080f0ab in cti_op_put_by_id at JITStubs.cpp:1351
#62	0x100803dcd in WTF::doubleHash at HashTable.h:447
#63	0x1007e21f7 in JSC::JITCode::execute at JITCode.h:77
#64	0x1007dcc56 in JSC::Interpreter::executeCall at Interpreter.cpp:851
#65	0x10079632d in JSC::call at CallData.cpp:38
#66	0x1022d2f41 in WebCore::JSMainThreadExecState::call at JSMainThreadExecState.h:48
#67	0x101c50447 in WebCore::JSEventListener::handleEvent at JSEventListener.cpp:123
#68	0x1018ed642 in WebCore::EventTarget::fireEventListeners at EventTarget.cpp:354
#69	0x1018edc71 in WebCore::EventTarget::fireEventListeners at EventTarget.cpp:323
#70	0x101ec7d8f in WebCore::Node::handleLocalEvents at Node.cpp:2543
#71	0x1018d78d6 in WebCore::EventContext::handleLocalEvents at EventContext.cpp:48
#72	0x101ecff58 in WebCore::Node::dispatchGenericEvent at Node.cpp:2694
#73	0x101ed0379 in WebCore::Node::dispatchEvent at Node.cpp:2612
#74	0x101eceb58 in WebCore::Node::dispatchMouseEvent at Node.cpp:2902
#75	0x101ecf183 in WebCore::Node::dispatchMouseEvent at Node.cpp:2799
#76	0x1018df9af in WebCore::EventHandler::dispatchMouseEvent at EventHandler.cpp:1910
#77	0x1018e0f90 in WebCore::EventHandler::handleMouseReleaseEvent at EventHandler.cpp:1623
#78	0x1018e6fd5 in WebCore::EventHandler::mouseUp at EventHandlerMac.mm:546
#79	0x100ff8b3c in -[WebHTMLView mouseUp:] at WebHTMLView.mm:3764
#80	0x7fff8568d7ed in -[NSWindow sendEvent:]
#81	0x100041bf5 in ??
#82	0x100041b81 in ??
#83	0x7fff855c2ee2 in -[NSApplication sendEvent:]
#84	0x1000388c2 in ??
#85	0x7fff85559922 in -[NSApplication run]
#86	0x7fff855525f8 in NSApplicationMain
#87	0x100009b18 in ??

[Rik Cabanier]

It's worse. Anything with table that have a percent will fail in debug mode.
I will fix this.

[Rik Cabanier]

Created attachment 83962 [details]
changed code to work around asserts + updated to real boolean values

[Rik Cabanier]

Created attachment 83963 [details]
changed code to work around asserts + updated to real boolean values

[Rik Cabanier]

Created attachment 83964 [details]
changed code to work around asserts + updated to real boolean values

[Adam Barth]

Comment on attachment 83964 [details]
changed code to work around asserts + updated to real boolean values

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

This change needs a test.

> Source/WebCore/ChangeLog:11
> +        * WebCore.xcodeproj/project.pbxproj:

Probably should remove this line from the changelog.

[Rik Cabanier]

Created attachment 83966 [details]
changed code to work around asserts + updated to real boolean values + added test

[Pavel Feldman]

Comment on attachment 83966 [details]
changed code to work around asserts + updated to real boolean values + added test

I don't see a test in this patch,

[Rik Cabanier]

(In reply to comment #7)
> (From update of attachment 83966 [details])
> I don't see a test in this patch,

The changelog is referring to a test file. Since this bug only happens in debug, I can't come up with a test case for the layout test since they don't run in debug.

http://www.webkit.org/coding/contributing.html doesn't mention anything about creating testfiles. http://www.webkit.org/quality/testwriting.html does but that is if you change the layout which this code doesn't.

[Adam Roben (:aroben)]

*** Bug 55321 has been marked as a duplicate of this bug. ***

[James Robinson]

Layout tests can (and are) run in debug, which is why so many bots are having trouble with the previous patch.

Why is it valid to remove this ASSERT()?  Did it serve no purpose before?  It looks like the table layout code was using setRawValue() when it wanted to directly set a value (and not go through this path), why did you change those to setValue()?

[Rik Cabanier]

(In reply to comment #10)
> Layout tests can (and are) run in debug, which is why so many bots are having trouble with the previous patch.
> 
> Why is it valid to remove this ASSERT()?  Did it serve no purpose before?  It looks like the table layout code was using setRawValue() when it wanted to directly set a value (and not go through this path), why did you change those to setValue()?

I did that by request of dhyatt since at first glance there was no more difference between setValue and setRawValue and he thought it was less confusing than having 2 almost identical calls.

These changes are definitely a work in progress as the intent is to move everything to float values at which point all the assert can go away.

[Rik Cabanier]

(In reply to comment #10)
> Layout tests can (and are) run in debug, which is why so many bots are having trouble with the previous patch.
> 
> Why is it valid to remove this ASSERT()?  Did it serve no purpose before?  It looks like the table layout code was using setRawValue() when it wanted to directly set a value (and not go through this path), why did you change those to setValue()?

In response to question 1 and 2:
the assert used to test that you didn't call setvalue with a percent value but SetRawValue. I guess this was done to make sure that you knew what your are doing (which seems hacky).

[Antti Koivisto]

Comment on attachment 83966 [details]
changed code to work around asserts + updated to real boolean values + added test

ChangeLog totally fails to explain the change but it does look correct. r+ anyway since this breaking stuff.

[Antti Koivisto]

Comment on attachment 83966 [details]
changed code to work around asserts + updated to real boolean values + added test

I take that back. This clearly changes the behavior of the quirks flag. It doesn't get reset anymore. Why is that ok?

[Adam Roben (:aroben)]

We're rolling out the offending change.

*** This bug has been marked as a duplicate of bug 55386 ***