Closed Bug 1155412 Opened 9 years ago Closed 9 years ago

Converting nsTableOuterFrame::OuterBeginReflowChild() placement-new reflow state to use Maybe<>

Categories

(Core :: Layout: Tables, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(1 file, 1 obsolete file)

nsTableOuterFrame::OuterBeginReflowChild() uses a somewhat-iffy placement-new strategy to create its reflow state.

Essentially, it looks like the caller doesn't have all the information needed to set up the reflow state, so it lets OuterBeginReflowChild() instantiate it; BUT, the caller needs to *use* the reflow state after calling OuterBeginReflowChild(). So right now, the caller sets aside some stack space, and lets OuterBeginReflowChild instantiate a reflowstate in that stack-space,  and then the caller uses it and cleans it up.

Anyway -- this would be clearer & safer with Maybe<nsHTMLReflowState>. Filing this bug on converting the OuterBeginReflowChild placement-new stuff to use Maybe<>.
With this patch, the caller creates a Maybe<> and passes that in, instead of passing in some stack-space.

Since Maybe<> essentially behaves like a pointer, this "just works" on the caller's side, with one exception -- the call to FinishReflowChild, which actually requires a pointer as its argument. Maybe<> doesn't implicitly convert to a pointer, so we have to call .ptr() on it in that spot for the function-argument.
Attachment #8593642 - Flags: review?(dbaron)
Also, the "reset()" calls are technically unnecessary; they're just there to preserve prior behavior.

(And to enforce a "we should not be using this anymore" rule for these objects, in debug builds at least, since any usages would trip Maybe<> assertions.)
I caught a bug when actually testing this (just viewing e.g. data:text/html,<table> ) -- it turns out we've been using captionRS uninitialized all this time.  It's been harmless, but now it trips a Maybe<> fatal assertion in debug builds.

We instantiate captionMet like so, using captionRS:
> 904   nsHTMLReflowMetrics captionMet(captionRS->GetWritingMode());
and at this point, captionRS may or may not have ever been constructed.

MXR Link:
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableOuterFrame.cpp?rev=41c005e9398e#903

I don't think this involves any virtual method calls, so it's not dangerous/exploitable -- we just end up with a potentially-bogus writing mode in captionMet (and only in cases where captionMet itself goes unused).

Anyway, there's a simple fix to prevent this uninitialized usage; just delay the construction of captionMet itself -- only construct it inside of the condition where we use it. (at which point we know captionRS has been initialized)

We still have to declare captionMet at the top level, though -- and hence we need it to be Maybe<> -- because we use it in two separate places, inside of two separate checks for "if (mCaptionFrames.NotEmpty()) {".

Carrying forward r+ since this is a small change & a logical extension of the existing patch.
Attachment #8593642 - Attachment is obsolete: true
Attachment #8594068 - Flags: review+
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/4606625aa995
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: