Closed Bug 1377978 Opened 2 years ago Closed 2 years ago

nsRange should use uint32_t for the start/end offsets (to match the spec)

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mats, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We're currently using int32_t but I think it would be good to change this
to uint32_t to match the spec:
https://dom.spec.whatwg.org/#interface-range
Blocks: 1377981
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
ok, need to be careful that we don't start to index out of bound of some arrays here or anything like that.
Comment on attachment 8886176 [details]
Bug 1377978 - Make nsRange use uint32_t to offset

https://reviewboard.mozilla.org/r/156976/#review162218

::: commit-message-d1e74:3
(Diff revision 1)
> +Bug 1377978 - Make nsRange use uint32_t to offset r?smaug
> +
> +DOM4 defines that offset of Range is unsigned long.  However, nsRange uses int32_t to them.

We don't try to implement DOM4. We should follow https://dom.spec.whatwg.org/

::: dom/base/nsContentUtils.h:408
(Diff revision 1)
>     *  0 if error or if point1 == point2.
>     *  NOTE! If the two nodes aren't in the same connected subtree,
>     *  the result is 1, and the optional aDisconnected parameter
>     *  is set to true.
>     */
> -  static int32_t ComparePoints(nsINode* aParent1, int32_t aOffset1,
> +  static int32_t ComparePoints(nsINode* aParent1, int64_t aOffset1,

This looks odd. Why suddenly 64 bit integers?

::: dom/base/nsRange.h:192
(Diff revision 1)
>    {
>      MOZ_ASSERT(aNode);
>      MOZ_ASSERT(aOffset);
>      nsINode* parentNode = aNode->GetParentNode();
> -    *aOffset = parentNode ? parentNode->IndexOf(aNode) : -1;
> -    if (*aOffset >= 0) {
> +    if (!parentNode) {
> +      return nullptr;

Please assign some value to aOffset when returning null. I don't like possibly uninitialized return values (out params)

::: dom/base/nsRange.h:196
(Diff revision 1)
> -    if (*aOffset >= 0) {
> +      return nullptr;
> -      (*aOffset)++;
>      }
> +    int32_t indexInParent = parentNode->IndexOf(aNode);
> +    if (NS_WARN_IF(indexInParent < 0)) {
> +      return nullptr;

same here.
I guess *aOffset = 0;

::: dom/base/nsRange.h:207
(Diff revision 1)
>    {
>      MOZ_ASSERT(aNode);
>      MOZ_ASSERT(aOffset);
>      nsINode* parentNode = aNode->GetParentNode();
> -    *aOffset = parentNode ? parentNode->IndexOf(aNode) : -1;
> +    if (!parentNode) {
> +      return nullptr;

Same here

::: dom/base/nsRange.h:210
(Diff revision 1)
> +    if (NS_WARN_IF(indexInParent < 0)) {
> +      return nullptr;

and here

::: dom/base/nsRange.h:374
(Diff revision 1)
> -  static bool IsValidOffset(nsINode* aNode, int32_t aOffset);
>  
> +  /**
> +   * XXX nsRange should accept 0 - UINT32_MAX as offset.  However, users of
> +   *     nsRange treat offset as int32_t.  Additionally, some other internal
> +   *     APIs like nsINode::IndexOf() still use int32_t.  Therefore, nsRange

"still". I would expect IndexOf to always use int32_t

::: dom/base/nsRange.h:378
(Diff revision 1)
> +   *     nsRange treat offset as int32_t.  Additionally, some other internal
> +   *     APIs like nsINode::IndexOf() still use int32_t.  Therefore, nsRange
> +   *     should accept only 0 - INT32_MAX as valid offset.
> +   */
> +  /* static */
> +  static bool IsValidOffset(int64_t aOffset)

why int64_t ?

::: dom/base/nsRange.cpp:106
(Diff revision 1)
>  
>    if (!aRange || !aRange->IsPositioned())
>      return NS_ERROR_UNEXPECTED;
>  
>    // gather up the dom point info
> -  int32_t nodeStart, nodeEnd;
> +  int64_t nodeStart, nodeEnd;

why int64_t

::: dom/base/nsRange.cpp:655
(Diff revision 1)
>  
>    nsINode* container = NODE_FROM(aContainer, aDocument);
>  
>    // Adjust position if a sibling was inserted.
> -  if (container == mStartContainer && aIndexInContainer < mStartOffset &&
> +  if (container == mStartContainer &&
> +      aIndexInContainer < static_cast<int64_t>(mStartOffset) &&

again int64_t
This is getting confusing.
I thought it is guaranteed that m*Offset fits into non-negative side of int32_t
Attachment #8886176 - Flags: review?(bugs) → review-
Comment on attachment 8886176 [details]
Bug 1377978 - Make nsRange use uint32_t to offset

https://reviewboard.mozilla.org/r/156976/#review162218

> We don't try to implement DOM4. We should follow https://dom.spec.whatwg.org/

Ah, okay. I checked it, and I reached it from a link in a spec whose label is DOM4!

I'll modify it to "DOM Standard".

> This looks odd. Why suddenly 64 bit integers?

nsContentUtils::ComparePoints() doesn't mind even if it gets negative offset. I guess that this is a bug, but some users depends on this odd behavior. Therefore, for making it accepting both uint32_t values and negative value, they need to be int64_t for now.

I'll explain with comment in nsContentUtils.h and add XXX comment to nsContentUtils.cpp.

> why int64_t ?

Ah, this can be uint32_t now with only changing nsRange::SelectNode().

> why int64_t

The reason is, it may contain the result of nsINode::IndexOf(). And there is a case actually set to -1. Additionally, they go to nsContentUtils::ComparePoints().

Okay, I'll add comment about the reason.

> again int64_t
> This is getting confusing.
> I thought it is guaranteed that m*Offset fits into non-negative side of int32_t

Yes. But aIndexInContainer isn't so. (If not, why it's "signed" integer?)

Since aIndexInContent might be negative value, and mStartOffset is uint32_t, then, we need to compare them as signed 64 bit integer, isn't it?

So, should I do like this?:

> NS_WARN_IF(aIndexInContainer < 0) &&
> static_cast<uint32_t>(aIndexInContainer) < mStartOffset &&

This is redundant, but might look better since it warns unexpected case.
I'll review this tomorrow, but I still don't understand the 64bit usage.
Elsewhere we can live with int32_t. And nsINode::IndexOf returns int32_t as an example,
and the methods take int32_t now.
(In reply to Olli Pettay [:smaug] from comment #11)
> I'll review this tomorrow, but I still don't understand the 64bit usage.
> Elsewhere we can live with int32_t. And nsINode::IndexOf returns int32_t as
> an example,
> and the methods take int32_t now.

The purpose of using int64_t in nsContentUtils::ComparePoints() and nsRange::CompareNodeToRange() is, they might be set to over INT32_MAX or -1. E.g., some callers in nsRange of nsContentUtils::ComparePoints() use mStartOffset and mEndOffset (uint32_t), but some callers of them initialize the argument with nsINode::IndexOf() and don't ban -1 before calling. Although, it should be rare case, even with current code, over INT32_MAX value may be used with nsContentUtils::ComparePoints() and which may cause unexpected result due to over INT32_MAX value is converted to negative integer. So, at least fixing the odd behavior, we should make nsContentUtils can treat both negative integer and over INT32_MAX value.  Then, we need to make them int64_t.

FYI: nodeEnd in nsRange::CompareNodeToRange() may be over INT32_MAX because the type of result of nsINode::GetChildCount() is uint32_t and even in the else case, it may be INT32_MAX + 1.
I'm still having hard time to understand. Currently we can live with int32_t (not uint32_t), how come we suddenly need int64_t?
INT32_MAX or -1 both fit into int32_t.
FWIW, currently a node can never have anywhere close to INT32_MAX child nodes.
And since we limit Range anyhow to INT32_MAX, why we need int64_t?
I think I'm missing something here. How is the int64_t handling related to this bug?

If nsRange::CompareNodeToRange is the only reason for int64_t usage, I don't think that warrants int64_t usage. remember that currently we limit number of child nodes:
http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/base/nsAttrAndChildArray.h#39

Though, bug 651120 may change that. But can we deal with that change separately.
Comment on attachment 8886176 [details]
Bug 1377978 - Make nsRange use uint32_t to offset

https://reviewboard.mozilla.org/r/156976/#review163828

still r- since for some reason int64_t usage feels really confusing to me. I can't figure out how this patch requires that change.
Attachment #8886176 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #13)
> I'm still having hard time to understand. Currently we can live with int32_t
> (not uint32_t), how come we suddenly need int64_t?
> INT32_MAX or -1 both fit into int32_t.
> FWIW, currently a node can never have anywhere close to INT32_MAX child
> nodes.

(In reply to Olli Pettay [:smaug] from comment #14)
> And since we limit Range anyhow to INT32_MAX, why we need int64_t?
> I think I'm missing something here. How is the int64_t handling related to
> this bug?
> 
> If nsRange::CompareNodeToRange is the only reason for int64_t usage, I don't
> think that warrants int64_t usage. remember that currently we limit number
> of child nodes:
> http://searchfox.org/mozilla-central/rev/
> a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/base/nsAttrAndChildArray.h#39

Oh, that's good information. Then, the patch can be without the change. But I wonder, why does nsINode::GetChildCount() uses uint32_t?
well, uint32_t nicely tells to the caller that it won't ever return < 0, which makes sense for child count.
Comment on attachment 8886176 [details]
Bug 1377978 - Make nsRange use uint32_t to offset

https://reviewboard.mozilla.org/r/156976/#review164088

::: dom/base/nsContentUtils.cpp:2777
(Diff revision 3)
>  nsContentUtils::ComparePoints(nsINode* aParent1, int32_t aOffset1,
>                                nsINode* aParent2, int32_t aOffset2,
>                                bool* aDisconnected)
>  {
>    if (aParent1 == aParent2) {
> +    // XXX This is odd.  aOffset1 and/or aOffset2 may be -1, e.g., it's result

Well, isn't this about anonymous content.

::: dom/base/nsRange.cpp:715
(Diff revision 3)
>    bool gravitateEnd = false;
>    bool didCheckStartParentDescendant = false;
>  
>    // Adjust position if a sibling was removed...
>    if (container == mStartContainer) {
> -    if (aIndexInContainer < mStartOffset) {
> +    if (aIndexInContainer < static_cast<int64_t>(mStartOffset)) {

Why int64_t?

::: dom/base/nsRange.cpp:726
(Diff revision 3)
>        nsContentUtils::ContentIsDescendantOf(mStartContainer, aChild);
>    }
>  
>    // Do same thing for end boundry.
>    if (container == mEndContainer) {
> -    if (aIndexInContainer < mEndOffset) {
> +    if (aIndexInContainer < static_cast<int64_t>(mEndOffset)) {

why int64_t?

::: dom/base/nsRange.cpp:1317
(Diff revision 3)
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  int32_t offset = -1;
> +  // If the node is being removed from its parent, GetContainerAndOffsetBefore()
> +  // returns nullptr.  Then, SetStart() will throw
> +  // NS_ERROR_DOM_INVALID_NODE_TYPE_ERR.
> +  uint32_t offset;

Please initiate offset to some value.

::: dom/base/nsRange.cpp:1356
(Diff revision 3)
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  int32_t offset = -1;
> +  // If the node is being removed from its parent, GetContainerAndOffsetAfter()
> +  // returns nullptr.  Then, SetStart() will throw
> +  // NS_ERROR_DOM_INVALID_NODE_TYPE_ERR.
> +  uint32_t offset;

ditto

::: dom/base/nsRange.cpp:1521
(Diff revision 3)
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  int32_t offset = -1;
> +  // If the node is being removed from its parent, GetContainerAndOffsetBefore()
> +  // returns nullptr.  Then, SetEnd() will throw
> +  // NS_ERROR_DOM_INVALID_NODE_TYPE_ERR.
> +  uint32_t offset;

ditto

::: dom/base/nsRange.cpp:1560
(Diff revision 3)
>  
>    AutoInvalidateSelection atEndOfBlock(this);
> -  int32_t offset = -1;
> +  // If the node is being removed from its parent, GetContainerAndOffsetAfter()
> +  // returns nullptr.  Then, SetEnd() will throw
> +  // NS_ERROR_DOM_INVALID_NODE_TYPE_ERR.
> +  uint32_t offset;

ditto

::: editor/libeditor/HTMLEditRules.cpp:5359
(Diff revision 3)
>        // endpoint is just after the close of a block.
>        nsCOMPtr<nsIDOMNode> child =
>          GetAsDOMNode(mHTMLEditor->GetRightmostChild(wsEndObj.mStartReasonNode,
>                                                      true));
>        if (child) {
> -        newEndNode = EditorBase::GetNodeLocation(child, &newEndOffset);
> +        int32_t offset;

Please initialize offset to some value

::: editor/libeditor/HTMLEditRules.cpp:5372
(Diff revision 3)
>        nsCOMPtr<nsIDOMNode> child;
>        NS_ENSURE_STATE(mHTMLEditor);
> -      mHTMLEditor->GetPriorHTMLNode(endNode, endOffset, address_of(child));
> +      mHTMLEditor->GetPriorHTMLNode(endNode, static_cast<int32_t>(endOffset),
> +                                    address_of(child));
>        if (child) {
> -        newEndNode = EditorBase::GetNodeLocation(child, &newEndOffset);
> +        int32_t offset;

ditto

::: editor/libeditor/HTMLEditRules.cpp:5406
(Diff revision 3)
>        // startpoint is just before the start of a block.
>        nsCOMPtr<nsIDOMNode> child =
>          GetAsDOMNode(mHTMLEditor->GetLeftmostChild(wsStartObj.mEndReasonNode,
>                                                     true));
>        if (child) {
> -        newStartNode = EditorBase::GetNodeLocation(child, &newStartOffset);
> +        int32_t offset;

ditto

::: editor/libeditor/HTMLEditRules.cpp:5418
(Diff revision 3)
>        nsCOMPtr<nsIDOMNode> child;
>        NS_ENSURE_STATE(mHTMLEditor);
> -      mHTMLEditor->GetNextHTMLNode(startNode, startOffset, address_of(child));
> +      mHTMLEditor->GetNextHTMLNode(startNode, static_cast<int32_t>(startOffset),
> +                                   address_of(child));
>        if (child) {
> -        newStartNode = EditorBase::GetNodeLocation(child, &newStartOffset);
> +        int32_t offset;

and here and elsewhere
Attachment #8886176 - Flags: review?(bugs) → review+
Comment on attachment 8886176 [details]
Bug 1377978 - Make nsRange use uint32_t to offset

https://reviewboard.mozilla.org/r/156976/#review164088

> Well, isn't this about anonymous content.

Oh, if the node is an anonymous content, does nsINode::IndexOf() return -1? I saw some oranges with initializing <input> element.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9c0436b452d6
Make nsRange use uint32_t to offset r=smaug
https://hg.mozilla.org/mozilla-central/rev/9c0436b452d6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.