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)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
Details
Attachments
(1 file, 1 obsolete file)
13.72 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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<>.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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.)
Attachment #8593642 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
Try run w/ fix v2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c51ac21bcb0f
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite-
Comment 6•9 years ago
|
||
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.
Description
•