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

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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)

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa232d72169

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6aa232d72169