Closed Bug 1405605 Opened 3 years ago Closed 3 years ago

One of the test-cases for bug 1404324 can make the IsReallyFixedPos assertion fire

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: assertion)

Attachments

(1 file)

The assertion in http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/layout/base/nsLayoutUtils.cpp#7378 can fire in one of the test-cases of bug 1404324. I overlooked that the test harness would complain about it.

It's not a correctness issue (indeed that's the point of the changes in bug 1404324).

We can remove the assertion (not great, because it seems useful), relax it (not great either), or just inline the same check on the code that can trigger it (nsPlaceholderFrame::RemoveFrom).

Didn't want to take any of the approaches without opinion first, since there are tradeoffs for all of them. For now I marked the assert as expected.

Another option is to live with the asserts() annotation, ofc.
Boris, wdyt?
Flags: needinfo?(bzbarsky)
Comment on attachment 8915086 [details]
Bug 1405605: Bypass the IsReallyFixedPos assertion when the style may have changed due to first-line reparenting.

https://reviewboard.mozilla.org/r/186352/#review191356


C/C++ static analysis didn't find any defects in this patch. Hooray!
Comment on attachment 8915086 [details]
Bug 1405605: Bypass the IsReallyFixedPos assertion when the style may have changed due to first-line reparenting.

https://reviewboard.mozilla.org/r/186352/#review191482

r=me

::: layout/base/nsLayoutUtils.cpp:7403
(Diff revision 1)
>  
> +/* static */ bool
> +nsLayoutUtils::MayBeReallyFixedPos(const nsIFrame* aFrame)
> +{
> +  MOZ_ASSERT(aFrame->GetParent(),
> +             "IsReallyFixedPos called on frame not in tree");

MayBeReallyFixedPos, in assert message.
Attachment #8915086 - Flags: review?(bzbarsky) → review+
Thanks for the review Boris.
Flags: needinfo?(bzbarsky)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c41d2b37dbf1 -d 66452b85b5df: rebasing 424122:c41d2b37dbf1 "Bug 1405605: Bypass the IsReallyFixedPos assertion when the style may have changed due to first-line reparenting. r=bz" (tip)
merging layout/base/nsLayoutUtils.cpp
merging layout/generic/nsPlaceholderFrame.cpp
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/generic/nsPlaceholderFrame.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8579323603
Bypass the IsReallyFixedPos assertion when the style may have changed due to first-line reparenting. r=bz
Comment on attachment 8915086 [details]
Bug 1405605: Bypass the IsReallyFixedPos assertion when the style may have changed due to first-line reparenting.

https://reviewboard.mozilla.org/r/186352/#review191490


C/C++ static analysis didn't find any defects in this patch. Hooray!
https://hg.mozilla.org/mozilla-central/rev/0c8579323603
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.