1381710 - Pass editing/run/backcolor.html on web-platform-tests when enabling lazy frame construction for editable content

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Makoto Kato [:m_kato]]

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 1

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=334b4839e0e0f700ba082fdd8fb3ba0360caa5ae

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1381710 - Selection.addRange will be failure with dynamic table insertion.

When range is selected table element, Selection.addRange uses nsFrameSelection.  If frame isn't constructed yet, addRange throws NS_ERROR_FAILURE even if table element isn't editable element.

When getting nsITableCellLayout, we should flush frame to construct cell frame.

Review commit: https://reviewboard.mozilla.org/r/158216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158216/

Comment 3

[Timothy Nikkel (:tnikkel)]

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?

Comment 4

[Makoto Kato [:m_kato]]

(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?

Comment 5

[Timothy Nikkel (:tnikkel)]

(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.

Comment 6

[Makoto Kato [:m_kato]]

(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.

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8887368 [details]
Bug 1381710 - Selection.addRange will be failure with dynamic table insertion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158216/diff/1-2/

Comment 8

[Makoto Kato [:m_kato]]

(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 9

[Timothy Nikkel (:tnikkel)]

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 10

[Makoto Kato [:m_kato]]

Comment on attachment 8887368 [details]
Bug 1381710 - Selection.addRange will be failure with dynamic table insertion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158216/diff/2-3/

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8887368 [details]
Bug 1381710 - Selection.addRange will be failure with dynamic table insertion.

https://reviewboard.mozilla.org/r/158216/#review172058

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4751560ba211
Selection.addRange will be failure with dynamic table insertion. r=tnikkel

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4751560ba211