Closed Bug 1245406 Opened 4 years ago Closed 4 years ago

Use mfbt::Maybe & "reset" to destroy/recreate nsHTMLReflowState on the stack, in nsBlockFrame::ReflowBlockFrame

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

nsBlockFrame::ReflowBlockFrame *directly* calls a destructor on a stack-allocated object, and then uses placement new to reconstruct it:

> 3323     nsHTMLReflowState
> 3324       blockHtmlRS(aState.mPresContext, aState.mReflowState, frame,
> 3325                   availSpace.Size(wm).ConvertTo(frame->GetWritingMode(), wm));
[...]
> 3329     do {
[...]
> 3436       blockHtmlRS.~nsHTMLReflowState();
> 3437       new (&blockHtmlRS) nsHTMLReflowState(aState.mPresContext,
> 3438                            aState.mReflowState, frame,
> 3439                            availSpace.Size(wm).ConvertTo(
> 3440                              frame->GetWritingMode(), wm));
> 3441     } while (true);

Right now we absolutely depend on the fact that these two operations (destruction & placement new) happen back-to-back. If someone were to modify this code such that these lines aren't back-to-back, I'm pretty sure we could up invoking the blockHtmlRS destructor twice (once explicitly, once when the variable goes out of scope), and/or using its bogus contents between its destruction & its re-construction.  All of which would be bad.

This would be cleaner & more foolproof if we used Maybe<nsHTMLReflowState>, with reset() and emplace() to destroy & reconstruct the reflow state.  That prevents the possibility of double-destruction, and it'll make us assert fatally if we try to use the object between destruction & reconstruction.

There is a small cost to doing this -- reset() and emplace() each modify a helper-bool inside of the Maybe<> object (which tracks whether the object is valid or not).  This seems like a reasonable price to pay for the peace of mind / foolproofing that it gives us, I think.
Attached patch fix v1Splinter Review
Here's the fix.

dbaron, I see you've got the "no new reviews" flag set -- putting this in your needinfo queue as a hackaround (no rush).  I'm hoping you can review, since it's tweaking code that you recently added, and I want to be sure you're OK with it. (and it hopefully should be a pretty quick review)
Flags: needinfo?(dbaron)
Flags: needinfo?(dbaron)
Attachment #8715190 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6aa232d72169
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.