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)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
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)

trigger.html
635 bytes, text/html
Details
backtrace of assertion failure
15.72 KB, text/plain
Details
Bug 1401807 Part 1 - Give unconstrained block size when reflowing mPosterImage.
59 bytes, text/x-review-board-request
dholbert
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1401807 Part 2 - Add a crashtest.
59 bytes, text/x-review-board-request
dholbert
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1401807 Part 3 - Use a local reflow status when reflowing nsVideoFrame's children.
59 bytes, text/x-review-board-request
dholbert
: review+
Sylvestre
: approval-mozilla-beta+
Details
Attached file trigger.html — 
Testcase found while fuzzing mozilla-central rev a20de99fa3c1.
Flags: in-testsuite?
status-firefox57: --- → wontfix
status-firefox58: --- → affected
Priority: -- → P3
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)
Seems awful premature to call a bug that regressed less than a week ago "wontfix" for 57.
status-firefox57: wontfix → affected
(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.)
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?
(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 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 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 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 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+
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)
(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
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 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?
Comment on attachment 8912789 [details]
Bug 1401807 Part 2 - Add a crashtest.

See comment 23.
Attachment #8912789 - Flags: approval-mozilla-beta?
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 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+
Attachment #8912789 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8912790 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1405830
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: