Closed
Bug 1401807
Opened 7 years ago
Closed 7 years ago
Assertion failure: aStatus.IsEmpty() (Caller should pass a fresh reflow status!), at /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1091
Categories
(Core :: Layout: Block and Inline, defect, P3)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: jkratzer, Assigned: TYLin)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(5 files)
635 bytes,
text/html
|
Details | |
15.72 KB,
text/plain
|
Details | |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
Testcase found while fuzzing mozilla-central rev a20de99fa3c1.
Flags: in-testsuite?
Updated•7 years ago
|
Comment 1•7 years ago
|
||
INFO: Last good revision: b2bc48a42a394ce548cca585b193d8bc49feda91 INFO: First bad revision: f6c6f4f6bd3431d8d6d209ee9f89f08efec56e60 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b2bc48a42a394ce548cca585b193d8bc49feda91&tochange=f6c6f4f6bd3431d8d6d209ee9f89f08efec56e60
Blocks: 1341009
Has Regression Range: --- → yes
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(tlin)
Comment 2•7 years ago
|
||
Seems awful premature to call a bug that regressed less than a week ago "wontfix" for 57.
Comment 3•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2) > Seems awful premature to call a bug that regressed less than a week ago > "wontfix" for 57. FWIW, the failing assertion was added as part of the push in comment 1: https://hg.mozilla.org/integration/autoland/rev/c0b4af15d17a#l18.12 That patch removed a bunch of nsReflowStatus::Reset() calls, and replaced them with assertions that should prove that they're unnecessary. This seems to be one place where a Reset() call is necessary, though -- somewhere. (Or the equivalent of a Reset() call is necessary.)
Comment 4•7 years ago
|
||
Comment 5•7 years ago
|
||
So it looks like nsVideoFrame (which can have multiple children) shamefully reuses its own Reflow method's aStatus when reflowing each of its children. Really, we should declare a helper nsReflowStatus, inside of its "for (nsIFrame* child : mFrames) {" loop, and then do something useful with it to populate nsVideoFrame's own aStatus outparam. TYLin, mind taking this?
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #5) > So it looks like nsVideoFrame (which can have multiple children) shamefully > reuses its own Reflow method's aStatus when reflowing each of its children. > > Really, we should declare a helper nsReflowStatus, inside of its "for > (nsIFrame* child : mFrames) {" loop, and then do something useful with it to > populate nsVideoFrame's own aStatus outparam. Yes. We should use a local child reflow status to reflow each child. However, by the comment [1], it looks like nsVideoFrame is a widget consists of its three children. Perhaps nsVideoFrame is not splittable and its own aStatus has nothing to do with its children's? I wonder whether we can add MOZ_ASSERT(aStatus.IsEmpty(), "This type of frame can't be split."); at the end of nsVideoFrame::Reflow(). > TYLin, mind taking this? Sure. I can look into this. [1] https://dxr.mozilla.org/mozilla-central/rev/33b7b8e81b4befcba503c0e48cd5370aeb715085/layout/generic/nsVideoFrame.cpp#312-314
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Flags: needinfo?(tlin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25164b23fc7d0d483bd82c3e9e28509f803ec7f5
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8912111 [details] Bug 1401807 Part 1 - Give unconstrained block size when reflowing mPosterImage. https://reviewboard.mozilla.org/r/183486/#review188888 ::: layout/generic/nsVideoFrame.cpp:342 (Diff revision 1) > ReflowChild(imageFrame, aPresContext, kidDesiredSize, kidReflowInput, > - posterRenderRect.x, posterRenderRect.y, 0, aStatus); > + posterRenderRect.x, posterRenderRect.y, 0, childReflowStatus); So it looks like we end up just ignoring the contents of childReflowStatus, and that's kind of bad. Hypothetically, if it's incomplete, then this child might be expecting us to create a continuation for it. So really, we should: (1) Add a post-condition to assert that our child reflow status is fully-complete, like the one in nsNumberControlFrame here: MOZ_ASSERT(childStatus.IsFullyComplete(), "We gave our child unconstrained available block-size, " "so it should be complete"); https://dxr.mozilla.org/mozilla-central/rev/33b7b8e81b4befcba503c0e48cd5370aeb715085/layout/forms/nsNumberControlFrame.cpp#186-188 (2) Be sure that the availableSize that we pass to our child *does actually* have unconstrained block size -- i.e. do something like: availableSize.BSize(wm) = NS_UNCONSTRAINEDSIZE; (It looks like we already do this in the lower ReflowChild call here, but not for this first one.)
Attachment #8912111 -
Flags: review?(dholbert)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8912111 [details] Bug 1401807 Part 1 - Give unconstrained block size when reflowing mPosterImage. https://reviewboard.mozilla.org/r/183486/#review189352 r=me
Attachment #8912111 -
Flags: review?(dholbert) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8912789 [details] Bug 1401807 Part 2 - Add a crashtest. https://reviewboard.mozilla.org/r/184060/#review189354 r=me ::: commit-message-4745a:1 (Diff revision 1) > +Bug 1401807 Part 2 - Add a crashtests. Nit: drop the final "s" (should be singular "crashtest")
Attachment #8912789 -
Flags: review?(dholbert) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8912790 [details] Bug 1401807 Part 3 - Use a local reflow status when reflowing nsVideoFrame's children. https://reviewboard.mozilla.org/r/184062/#review189358 r=me
Attachment #8912790 -
Flags: review?(dholbert) → review+
Comment 16•7 years ago
|
||
TYLin, could we try to uplift this to 57 once it's landed on Nightly? (It's kind of logically part of bug 1341009, which made it to 57.)
Flags: needinfo?(tlin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #16) > TYLin, could we try to uplift this to 57 once it's landed on Nightly? > > (It's kind of logically part of bug 1341009, which made it to 57.) Sure. I'll uplift the three patches after they land on Nightly. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba1b2ba24315ed702b88821e1cd976c8b2cacf05
Comment 21•7 years ago
|
||
Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dce12df1e58d Part 1 - Give unconstrained block size when reflowing mPosterImage. r=dholbert https://hg.mozilla.org/integration/autoland/rev/6db5435a16a4 Part 2 - Add a crashtest. r=dholbert https://hg.mozilla.org/integration/autoland/rev/6fad485bf964 Part 3 - Use a local reflow status when reflowing nsVideoFrame's children. r=dholbert
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dce12df1e58d https://hg.mozilla.org/mozilla-central/rev/6db5435a16a4 https://hg.mozilla.org/mozilla-central/rev/6fad485bf964
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 23•7 years ago
|
||
Comment on attachment 8912111 [details] Bug 1401807 Part 1 - Give unconstrained block size when reflowing mPosterImage. Approval Request Comment [Feature/Bug causing the regression]: Bug 1341009. [User impact if declined]: No. (This is a debug-only assertion.) [Is this code covered by automated tests?]: Yes, with a crash test added. [Has the fix been verified in Nightly?]: Yes. Landed on Nigthly. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: Part 1 - Part 3 in this bug. [Is the change risky?]: Not risky. [Why is the change risky/not risky?]: This fixed a debug-only assertion, should not affect video frame's rendering. [String changes made/needed]: None.
Flags: needinfo?(tlin)
Attachment #8912111 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 24•7 years ago
|
||
Comment on attachment 8912789 [details] Bug 1401807 Part 2 - Add a crashtest. See comment 23.
Attachment #8912789 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 25•7 years ago
|
||
Comment on attachment 8912790 [details] Bug 1401807 Part 3 - Use a local reflow status when reflowing nsVideoFrame's children. See comment 23.
Attachment #8912790 -
Flags: approval-mozilla-beta?
Comment 26•7 years ago
|
||
Comment on attachment 8912111 [details] Bug 1401807 Part 1 - Give unconstrained block size when reflowing mPosterImage. Fix an assert issue, taking it. Should be in 57b5
Attachment #8912111 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8912789 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8912790 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 27•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/792634630d29 https://hg.mozilla.org/releases/mozilla-beta/rev/3c55cc6a76f2 https://hg.mozilla.org/releases/mozilla-beta/rev/d9f70ecf91d3
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•