Closed Bug 1331903 Opened 8 years ago Closed 8 years ago

-moz-binding is hiding test issues and browser bugs

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(3 files)

Bug 1331903 part 1 - Fix test_inherit_computation.html test for width and inline-size.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331903 part 2 - Do not add -moz-binding to gChildRule3 in test_inherit_computation.html.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1331903 part 3 - Force a recreation of the display subtree after testing each property in test_*_computation.html.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
When I was looking at a mochitest failure in stylo, I found that the result from stylo actually makes sense, so I started wondering why Gecko passes the test. And it turns out Gecko fails at the same place, but -moz-binding hides the issue.

To reproduce this issue:
1. open layout/style/test/test_inherit_computation.html
2. add "if (property == '-moz-binding') return;" at the beginning of function test_property()
3. run "./mach mochitest layout/style/test/test_inherit_computation.html"

I see there are now 12 failures rather than 0.

Alternatively, if I
1. open layout/style/test/property_database.js
2. remove the information of "-moz-binding"
3. run "./mach mochitest layout/style/test/test_inherit_computation.html"

The test crashes with the following stack:
> Assertion failure: mType == StyleShapeSourceType::Box || mType == StyleShapeSourceType::Shape (Wrong shape source type!), at layout\style\nsStyleStruct.h:2752
> #01: nsFloatManager::AddFloat (layout\generic\nsfloatmanager.cpp:277)
> #02: mozilla::BlockReflowInput::FlowAndPlaceFloat (layout\generic\blockreflowinput.cpp:994)
> #03: mozilla::BlockReflowInput::AddFloat (layout\generic\blockreflowinput.cpp:630)
> #04: nsLineLayout::ReflowFrame (layout\generic\nslinelayout.cpp:981)
> #05: nsInlineFrame::ReflowInlineFrame (layout\generic\nsinlineframe.cpp:807)
> #06: nsInlineFrame::ReflowFrames (layout\generic\nsinlineframe.cpp:690)
> #07: nsInlineFrame::Reflow (layout\generic\nsinlineframe.cpp:468)
> #08: nsLineLayout::ReflowFrame (layout\generic\nslinelayout.cpp:940)
> #09: nsBlockFrame::ReflowInlineFrame (layout\generic\nsblockframe.cpp:4159)
> #10: nsBlockFrame::DoReflowInlineFrames (layout\generic\nsblockframe.cpp:3959)
> #11: nsBlockFrame::ReflowInlineFrames (layout\generic\nsblockframe.cpp:3836)
> #12: nsBlockFrame::ReflowLine (layout\generic\nsblockframe.cpp:2840)
> #13: nsBlockFrame::ReflowDirtyLines (layout\generic\nsblockframe.cpp:2376)
> #14: nsBlockFrame::Reflow (layout\generic\nsblockframe.cpp:1250)
> #15: nsBlockReflowContext::ReflowBlock (layout\generic\nsblockreflowcontext.cpp:307)
> #16: nsBlockFrame::ReflowBlockFrame (layout\generic\nsblockframe.cpp:3474)
> #17: nsBlockFrame::ReflowLine (layout\generic\nsblockframe.cpp:2836)
> #18: nsBlockFrame::ReflowDirtyLines (layout\generic\nsblockframe.cpp:2376)
> #19: nsBlockFrame::Reflow (layout\generic\nsblockframe.cpp:1250)
> #20: nsBlockReflowContext::ReflowBlock (layout\generic\nsblockreflowcontext.cpp:307)
> #21: nsBlockFrame::ReflowBlockFrame (layout\generic\nsblockframe.cpp:3474)
> #22: nsBlockFrame::ReflowLine (layout\generic\nsblockframe.cpp:2836)
> #23: nsBlockFrame::ReflowDirtyLines (layout\generic\nsblockframe.cpp:2376)
> #24: nsBlockFrame::Reflow (layout\generic\nsblockframe.cpp:1250)
> #25: nsContainerFrame::ReflowChild (layout\generic\nscontainerframe.cpp:1033)
> #26: nsCanvasFrame::Reflow (layout\generic\nscanvasframe.cpp:681)
> #27: nsContainerFrame::ReflowChild (layout\generic\nscontainerframe.cpp:1033)
> #28: nsHTMLScrollFrame::ReflowScrolledFrame (layout\generic\nsgfxscrollframe.cpp:557)
> #29: nsHTMLScrollFrame::ReflowContents (layout\generic\nsgfxscrollframe.cpp:687)
> #30: nsHTMLScrollFrame::Reflow (layout\generic\nsgfxscrollframe.cpp:1041)
> #31: nsContainerFrame::ReflowChild (layout\generic\nscontainerframe.cpp:1076)
> #32: mozilla::ViewportFrame::Reflow (layout\generic\viewportframe.cpp:318)
> #33: mozilla::PresShell::DoReflow (layout\base\presshell.cpp:9129)
> #34: mozilla::PresShell::ProcessReflowCommands (layout\base\presshell.cpp:9296)
> #35: mozilla::PresShell::FlushPendingNotifications (layout\base\presshell.cpp:4133)
> #36: nsDocument::FlushPendingNotifications (dom\base\nsdocument.cpp:7816)
> #37: nsComputedDOMStyle::UpdateCurrentStyleSources (layout\style\nscomputeddomstyle.cpp:672)
> #38: nsComputedDOMStyle::GetPropertyCSSValue (layout\style\nscomputeddomstyle.cpp:830)
> #39: nsComputedDOMStyle::GetPropertyValue (layout\style\nscomputeddomstyle.cpp:402)
> #40: mozilla::dom::CSSStyleDeclarationBinding::getPropertyValue (obj-firefox-opt\dom\bindings\cssstyledeclarationbinding.cpp:165)
> #41: ??? (???:???)
It seems to me that, the test around -moz-binding drops #fparent from the frame tree, and never rebuild it. That says, #fparent and #fchild are effectively identical to #nparent and #nchild for properties after -moz-binding, which means we are not actually testing them in the way we expect, which seems to be bad.
(I tried to work out a reproducible testcase, but I failed. I guess there is some restriction on where -moz-binding can be used. I guess using -moz-binding in a normal web page wouldn't work at all, and thus would not trigger this issue.)
Ugh. Is there a way we can just test the moz-binding stuff last?
Depends on: 1332180
Assignee: nobody → xidorn+moz
Depends on: 1332193
Comment on attachment 8828293 [details]
Bug 1331903 part 1 - Fix test_inherit_computation.html test for width and inline-size.

https://reviewboard.mozilla.org/r/105756/#review106914
Attachment #8828293 - Flags: review?(cam) → review+
Comment on attachment 8828294 [details]
Bug 1331903 part 2 - Do not add -moz-binding to gChildRule3 in test_inherit_computation.html.

https://reviewboard.mozilla.org/r/105758/#review106918
Attachment #8828294 - Flags: review?(cam) → review+
Comment on attachment 8828295 [details]
Bug 1331903 part 3 - Force a recreation of the display subtree after testing each property in test_*_computation.html.

https://reviewboard.mozilla.org/r/105760/#review106920
Attachment #8828295 - Flags: review?(cam) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4d776c90a15
part 1 - Fix test_inherit_computation.html test for width and inline-size. r=heycam
https://hg.mozilla.org/integration/autoland/rev/dc783cbd753b
part 2 - Do not add -moz-binding to gChildRule3 in test_inherit_computation.html. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4261558b51c9
part 3 - Force a recreation of the display subtree after testing each property in test_*_computation.html. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: