-moz-binding is hiding test issues and browser bugs

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

a year ago
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: ??? (???:???)
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
(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?
(Assignee)

Updated

a year ago
Depends on: 1332180
(Assignee)

Updated

a year ago
Assignee: nobody → xidorn+moz
(Assignee)

Updated

a year ago
Depends on: 1332193
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
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 9

a year ago
mozreview-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 10

a year ago
mozreview-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+

Comment 11

a year ago
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

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d4d776c90a15
https://hg.mozilla.org/mozilla-central/rev/dc783cbd753b
https://hg.mozilla.org/mozilla-central/rev/4261558b51c9
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.