Closed
Bug 1377978
Opened 7 years ago
Closed 7 years ago
nsRange should use uint32_t for the start/end offsets (to match the spec)
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, 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
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a8c4e3f54166ad0430156d3bea8e7f60a2f3919
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15c4820995af86ce015e9678ecbb0c9fbbf36877
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d579310a78425c2e48e001e4a58337ef952093cd
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80ce44cca5bb6e0a9ae722fce896152ab201e7b9
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
ok, need to be careful that we don't start to index out of bound of some arrays here or anything like that.
Comment 7•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d40396da7aa2742968fcea1a0c506d458bab43bb
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Comment 13•7 years ago
|
||
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.
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 16•7 years ago
|
||
(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?
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b67e5d54a522d6620e8cec7de09d9c0679cea0a
Comment 18•7 years ago
|
||
well, uint32_t nicely tells to the caller that it won't ever return < 0, which makes sense for child count.
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 22•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bbe90844a442f1245f04681384e9afbf4382e64
Assignee | ||
Comment 23•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3ffaf0c35b0490da8f9b96c33e532132630acf
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/9c0436b452d6 Make nsRange use uint32_t to offset r=smaug
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9c0436b452d6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•