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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Boris, wdyt?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1405605: Bypass the IsReallyFixedPos assertion when the style may have changed due to first-line reparenting.

Review commit: https://reviewboard.mozilla.org/r/186352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186352/

Comment 3

[Code Review Bot [:reviewbot]]

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 4

[Boris Zbarsky [: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/#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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8915086 [details]
Bug 1405605: Bypass the IsReallyFixedPos assertion when the style may have changed due to first-line reparenting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186352/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Thanks for the review Boris.

Comment 7

[Mozilla Autoland]

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)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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 9

[Code Review Bot [:reviewbot]]

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!

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c8579323603