1353573 - nsFrame::BoxReflow manipulates frame state bits in ways that are likely unsafe

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In bug 1353187 I just tripped over (see "second interdiff to fix test failure" there, attachment 8854642 [details] [diff] [review]) this code in nsFrame::BoxReflow:

    nsFrameState savedState = parentFrame->GetStateBits();
    WritingMode parentWM = parentFrame->GetWritingMode();
    ReflowInput
      parentReflowInput(aPresContext, parentFrame, aRenderingContext,
                        LogicalSize(parentWM, parentSize),
                        ReflowInput::DUMMY_PARENT_REFLOW_STATE);
    parentFrame->RemoveStateBits(~nsFrameState(0));
    parentFrame->AddStateBits(savedState);


This code is pretty scary and doesn't reflect how everybody is assuming frame state bits work.  If a bit like NS_FRAME_EXTERNAL_REFERENCE were changed inside of this reflow input constructor, then we'd have sec-critical bugs pretty quickly.  I think similar things may be true of other bits.

I think we should figure out why this code is there and what bits it actually wants to mess with, and then make it not mess with any of the other bits.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't have time to deal with this before my travel, but it could use an assignee.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(It seems I probably wrote the code, too, as part of bug 300030.  Figuring out the exact history will require figuring out CVS, since the history of the branch wasn't imported to git...)

Comment 3

[Andrew McCreight [:mccr8]]

I'm going to assume this code is old.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like this is unlikely to get fixed in time for 53 release but I'll track it in case of 53 dot release.

Comment 5

[Mats Palmgren (inactive)]

It looks like this landed 2006-12-08:
http://searchfox.org/mozilla-central/diff/31f1898810f733c40bfe3fd4b25295885feb2e39/layout/generic/nsFrame.cpp#5984-5990
It seems the documentation is in the CVS branches where the work happened:
http://searchfox.org/mozilla-central/commit/31f1898810f733c40bfe3fd4b25295885feb2e39
I don't know if we still have those branches around somewhere.

In any case, here's the nsHTMLReflowState at the time of the landing:
http://searchfox.org/mozilla-central/rev/31f1898810f733c40bfe3fd4b25295885feb2e39/layout/generic/nsHTMLReflowState.cpp
It has only two AddStateBits calls:
http://searchfox.org/mozilla-central/rev/31f1898810f733c40bfe3fd4b25295885feb2e39/layout/generic/nsHTMLReflowState.cpp#156-157,314,321
NS_FRAME_IS_DIRTY and NS_FRAME_CONTAINS_RELATIVE_HEIGHT.
There's also a RemoveStateBits call on the latter.
(nsLayoutUtils.cpp at the time didn't have any Add/RemoveStateBits calls)

The call from nsFrame::BoxReflow is to the /other/ ctor though, which doesn't set
NS_FRAME_IS_DIRTY under any circumstances (and still doesn't):
http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/layout/generic/ReflowInput.cpp#56
so I guess this was about NS_FRAME_CONTAINS_RELATIVE_HEIGHT?

Comment 6

[Mats Palmgren (inactive)]

Let's see if removing it breaks anything:
https://hg.mozilla.org/try/rev/c57da552d428e6266b62d7581fb1d18699868136

Comment 7

[Mats Palmgren (inactive)]

An up-to-date ReflowInput.cpp has one new flag that it *potentially* adds/removes:
NS_FRAME_IN_CONSTRAINED_BSIZE (apart from NS_FRAME_HAS_PROPERTIES)
and nsLayoutUtils.cpp *potentially* adds:
NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE
so I don't think there is any security issue here currently.

It's /hypothetically/ an issue, if someone adds new Add/RemoveStateBits calls that
depends on not being clobbered, but I don't think the security rating is for such
potential future issues, so I'm changing this to sec-want.

Also, not much content can trigger this nsFrame::BoxReflow code path anyway (only
XUL content I think).

Comment 8

[Mats Palmgren (inactive)]

Attachment: Don't try to preserve frame state bits across the ReflowInput ctor.

The Try run looks green-ish, so it looks like this isn't needed anymore.

I'd guess that this is probably a safer default behavior to have here.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8855542 [details] [diff] [review]
Don't try to preserve frame state bits across the ReflowInput ctor.

Sounds good, and thanks for taking care of this.

Comment 10

[Jet Villegas (inactive)]

(In reply to David Baron :dbaron: ⌚️UTC+8 from comment #9)
> Sounds good, and thanks for taking care of this.

Indeed. Thanks, Mats.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/7d6d6e8ec1cc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Do we want/need to uplift this anywhere or let it ride the trains?

Comment 13

[Mats Palmgren (inactive)]

No need to uplift this IMO.  This code is very old and was only a potential pitfall
when adding new code to the ReflowInput ctor, like David did in bug 1353187 adding
the NS_FRAME_HAS_PROPERTIES bit.  Such changes are rare and very unlikely to occur
on branches.