Split from bug 1348073. When enabling lazy frame construction for editable content, editing/run/backcolor.html is always failed due to NS_ERROR_FAILURE. Because nsFrameSelection depends on frame when selecting table. But frame isn't constructed at this time.

Comment on attachment 8887368 [details] Bug 1381710 - Selection.addRange will be failure with dynamic table insertion. https://reviewboard.mozilla.org/r/158216/#review165022 Did you check that all places that call this are safe to expect a flush in this functio can destroy anything?

(In reply to Timothy Nikkel (:tnikkel) from comment #3) > Comment on attachment 8887368 [details] > Bug 1381710 - Selection.addRange will be failed with dynamic table insertion. > > https://reviewboard.mozilla.org/r/158216/#review165022 > > Did you check that all places that call this are safe to expect a flush in > this functio can destroy anything? GetCellLayout will call from (via HandleTableSelection or UnselectCells) - nsFrameSelection::TakeFocus for table - nsFrame::HandlePress for table - nsFrame::HandleDrag for table - a11y via nsFrameSelection::RemoveCellsFromSelection and nsFrameSelection::RestrictCellsToSelection If we should fix Selection.addRange case only like this, we should only add calling FlushPendingNotification in Selection::GetTableCellLocationFromRange. It will be enough to fix this by adding FlushPendingNotification on GetTableCellLocationFromRange. How do you think?

(In reply to Makoto Kato [:m_kato] from comment #4) > (In reply to Timothy Nikkel (:tnikkel) from comment #3) > > Comment on attachment 8887368 [details] > > Bug 1381710 - Selection.addRange will be failed with dynamic table insertion. > > > > https://reviewboard.mozilla.org/r/158216/#review165022 > > > > Did you check that all places that call this are safe to expect a flush in > > this functio can destroy anything? > > GetCellLayout will call from (via HandleTableSelection or UnselectCells) > - nsFrameSelection::TakeFocus for table > - nsFrame::HandlePress for table > - nsFrame::HandleDrag for table > - a11y via nsFrameSelection::RemoveCellsFromSelection and > nsFrameSelection::RestrictCellsToSelection > > If we should fix Selection.addRange case only like this, we should only add > calling FlushPendingNotification in > Selection::GetTableCellLocationFromRange. It will be enough to fix this by > adding FlushPendingNotification on GetTableCellLocationFromRange. How do > you think? I don't really know this code well enough to say what is better, I just want to make sure we're not adding potential UAF.

(In reply to Timothy Nikkel (:tnikkel) from comment #5) > I don't really know this code well enough to say what is better, I just want > to make sure we're not adding potential UAF. OK, current fix is impacted for all table selection, so I will change this for AddRange only.

(In reply to Timothy Nikkel (:tnikkel) from comment #5) > I don't really know this code well enough to say what is better, I just want > to make sure we're not adding potential UAF. Timothy, since mats is in vacation, who is better reviewer for this? This bug depends on qf:1.

Comment on attachment 8887368 [details] Bug 1381710 - Selection.addRange will be failure with dynamic table insertion. https://reviewboard.mozilla.org/r/158216/#review170202 ::: dom/base/Selection.cpp:662 (Diff revision 2) > + if (presShell) { > + presShell->FlushPendingNotifications(FlushType::Frames); > + } > + > //Note: This is a non-ref-counted pointer to the frame > nsITableCellLayout *cellLayout = mFrameSelection->GetCellLayout(child); We don't hold a ref to |child| so it could be dead after we flush. |mFrameSelection| could be null after we've flushed. |this| could go away after we've flushed. Selection::AddTableCellRange calls this function and it also doesn't seem to be prepared to handle flushing (ie |this| or |mFrameSelection| going away).

Comment on attachment 8887368 [details] Bug 1381710 - Selection.addRange will be failure with dynamic table insertion. https://reviewboard.mozilla.org/r/158216/#review172058

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/autoland/rev/4751560ba211 Selection.addRange will be failure with dynamic table insertion. r=tnikkel