1445929 - Editor(Raw)DOMPoint::IsEndOfContainer() doesn't work if container is a text node

Status: RESOLVED FIXED

(Core :: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

>  bool
>  IsEndOfContainer() const
>  {
> ...
>    if (mIsChildInitialized) {
>      if (!mChild) {
>        NS_WARNING_ASSERTION(!mOffset.isSome() ||
>                             mOffset.value() == mParent->Length(),
>          "If mOffset was initialized, it should be length of the container");
>        return true;
>      }
>      NS_WARNING_ASSERTION(!mOffset.isSome() ||
>                           mParent->GetChildAt_Deprecated(mOffset.value()) == mChild,
>        "If mOffset and mChild are mismatched");
>      return false;
>    }
>    MOZ_ASSERT(mOffset.isSome());
>    return mOffset.value() == mParent->Length();
>  }

mIsChildInitialized may be true even if mParent is a text node. Therefore, if mIsChildInitialized is true and mParent is a text node, this always returns true because mChild is always nullptr.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Unfortunately, I found this bug when I work on another bug. I'm not sure how to meet actual symptom.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Attachment: Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild

Editor(Raw)DOMPoint::IsEndOfContainer() checks mIsChildInitialized before
referring mChild.  However, it may be true even if mParent is not a container
node like a text node.  Therefore, if mParent is a text node and
mIsChildInitialized is true, it always returns true (i.e., even if mOffset
isn't same as length of mParent).

This patch makes it check mParent->IsContainerNode() before checking
mIsChildInitialized because after checking mIsChildInitialized, it validates
the relation of the members.  So, this keeps the validation simple.

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

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8959173 [details]
Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild

https://reviewboard.mozilla.org/r/228032/#review234072

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d25e14fec76
Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild r=m_kato

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5800617915bbfe3b299ee41c1e2e2ff5357fa949

Comment 7

[Raul Gurzau (:RaulGurzau)]

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

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

Comment on attachment 8959173 [details]
Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1419745.

[User impact if declined]:
Not sure, we don't have actual bug report.

The bug is, IsEndOfContainer() returns true if Editor(Raw)DOMPoint's container is text node or something other data nodes and offset is even not same as length of the text node. So, callers of IsEndOfContainer() may set the container to a text node, the check won't work as expected.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes since this bug blocked to fix bug 1445569.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Before IsEndOfContainer() does something complicated, this patch inserts an early-return block for non-container nodes such as text node.

[String changes made/needed]:
No.

Comment 9

[Julien Cristau [:jcristau]]

If there's no known user impact, and this is already on release, what's the downside of letting this ride the trains?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate)]

(In reply to Julien Cristau [:jcristau] from comment #9)
> If there's no known user impact, and this is already on release, what's the
> downside of letting this ride the trains?

We still don't have enough testcases for editor module. But this is actual regression of API in editor. So, callers of this method in editor assumes that it checks if the point is end of a node even when the node is a text node.

Unfortunately, there are a lot of callers:
https://dxr.mozilla.org/mozilla-beta/search?q=IsEndOfContainer&redirect=false
And some of them look treating DOM point in text node (e.g., EditorBase::DeleteSelectionAndPrepareToCreateNode(), HTMLEditRules::ReturnInParagraph()). So, I believe that in some situation, this bug causes something odd behavior.

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8959173 [details]
Bug 1445929 - Make Editor(Raw)DOMPoint::IsEndOfContainer() check if mParent is container node before checking mChild

editor regression fix, beta60+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ee4ed3b4b945