stylo: file_fullscreen-backdrop.html Assertion failure: backdropFrame->GetParent()->IsViewportFrame() for fullscreen test on Mac

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: xidorn, Assigned: emilio)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
I have no idea what's happening there... This assumption should be hold :/ I probably need to investigate what's happening there.
Priority: -- → P2
(Reporter)

Comment 1

7 months ago
I tried locally on my mac, and this assertion doesn't show up... so it is somehow try server only :/
(Reporter)

Updated

7 months ago
Priority: P2 → P3
Is this still happening?
Flags: needinfo?(xidorn+moz)
(Reporter)

Updated

6 months ago
Summary: stylo: Assertion failure: backdropFrame->GetParent()->IsViewportFrame() for fullscreen test on Mac → stylo: file_fullscreen-backdrop.html Assertion failure: backdropFrame->GetParent()->IsViewportFrame() for fullscreen test on Mac
(Reporter)

Comment 4

6 months ago
Oh, actually, this assertion is wrong. Backdrop frame can be parented by either the viewport frame or the canvas frame, depending on whether its position is "fixed" or "absolute". Related code can be found in nsCSSFrameConstructor.cpp [1].

The value of position is "fixed" by default, so this assertion doesn't show up on majority of tests, but in the specific file_fullscreen-backdrop.html test, we try to change the position property to "absolute" and test that it works as expected. In that case, the assertion could be triggered.

But it is not clear to me why this happens only in Mac, and why I cannot reproduce this crash locally.

emilio, do you have idea why it isn't always triggered, and what would go wrong if the assertion is violated?


[1] https://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/layout/base/nsCSSFrameConstructor.cpp#1052-1055
Flags: needinfo?(emilio)
(Reporter)

Comment 5

6 months ago
This makes me think that it probably should have a slightly higher priority.
Priority: P3 → P2
(Reporter)

Updated

6 months ago
Priority: P2 → P3
(Assignee)

Comment 6

6 months ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> emilio, do you have idea why it isn't always triggered, and what would go
> wrong if the assertion is violated?

No idea off-hand about why this isn't always triggered, I guess it's hard to trigger a restyle of this frame given -moz-top-layer changes also reframe?

Re. what can go wrong, actually nothing in this case (if it was other kind of parent we could be doing unnecessary work, or not). I'd like to keep it given it makes it easier to reason about it, let's relax it.
Flags: needinfo?(emilio)
Comment hidden (mozreview-request)
(Reporter)

Comment 8

6 months ago
mozreview-review
Comment on attachment 8906918 [details]
Bug 1387942: Relax backdrop frame parenthood assertion, and re-enable test.

https://reviewboard.mozilla.org/r/178654/#review183646

r=me with the test re-enabled at the same time.

::: commit-message-e21a0:4
(Diff revision 1)
> +Bug 1387942: Relax backdrop frame parenthood assertion. r?xidorn
> +
> +MozReview-Commit-ID: 9VP6LgqUZ1F
> +

Please also include the change from https://hg.mozilla.org/try/rev/174fd17c84478d0e7918664da5b2749c99ac7ed8 to re-enable this test.
Attachment #8906918 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)

Comment 10

6 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/874f4d1c9ba1
Relax backdrop frame parenthood assertion, and re-enable test. r=xidorn
(Reporter)

Updated

6 months ago
Assignee: nobody → emilio
https://hg.mozilla.org/mozilla-central/rev/874f4d1c9ba1
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.