Closed Bug 1149163 Opened 7 years ago Closed 7 years ago

Begin removing nsCOMArray from nsHTMLEditRules

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(9 files)

6.83 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
13.45 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
8.97 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
6.06 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
18.74 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
10.39 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
12.21 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
9.22 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
30.73 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
This time I'll get a green try run before attaching patches.  Just filing for the bug number.
I decided to leave aIndex a pointer with a MOZ_ASSERT instead of converting to reference, to avoid ambiguity at the call site about the fact that the param is in-out.
Attachment #8592753 - Flags: review?(ehsan)
For the MOZ_ASSERT(NS_SUCCEEDED(res))s from nsIContentIterator::Init, code inspection seems to indicate no actual failure path.
Attachment #8592756 - Flags: review?(ehsan)
Is this the right way to do this?
Attachment #8592762 - Flags: review?(ehsan)
Do we want a const version of GetAsDOMNode, or is const nsINode* uncommon enough that const_cast is good enough?
Attachment #8592766 - Flags: review?(ehsan)
I have a green try run, but I adjusted some of the patches in the course of submitting, so I'll get another one.
Attachment #8592775 - Flags: review?(ehsan)
Comment on attachment 8592775 [details] [diff] [review]
Part 9 - Clean up nsHTMLEditRules::GetNodesFromSelection

Review of attachment 8592775 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +6048,5 @@
>      NS_ENSURE_STATE(mHTMLEditor);
>      if (!mHTMLEditor->IsEditable(testNode))
>      {
> +      arrayOfNodes.RemoveElementAt(i);
> +      continue;

This "continue" is because previously we would try to remove a non-editable table element from the list twice, which would remove the next element.  The switch to nsTArray caught it in the case where the list ends with a non-editable table element, because then it tried to remove beyond the end of the list, and nsTArray asserts in that case.
Comment on attachment 8592753 [details] [diff] [review]
Part 1 - Clean up nsHTMLEditRules::GetInnerContent

Nathan kindly agreed to take some of these reviews from me.  Nathan, please feel free to bounce back if you don't feel comfortable with the reviews.  Thanks!
Attachment #8592753 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592756 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592759 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592762 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592766 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592767 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592769 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592772 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8592775 - Flags: review?(ehsan) → review?(nfroyd)
Comment on attachment 8592753 [details] [diff] [review]
Part 1 - Clean up nsHTMLEditRules::GetInnerContent

Review of attachment 8592753 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks in advance for putting up with stupid questions.

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +12,5 @@
>  #include "mozilla/MathAlgorithms.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/dom/Selection.h"
>  #include "mozilla/dom/Element.h"
> +#include "mozilla/dom/OwningNonNull.h"

I guess you really need this in a later patch?

@@ +5176,5 @@
> +  NS_ENSURE_STATE(node);
> +  nsTArray<nsCOMPtr<nsINode>> outArrayOfNodes;
> +  for (int32_t i = 0; i < outArrayOfDOMNodes.Count(); i++) {
> +    nsCOMPtr<nsINode> newNode = do_QueryInterface(outArrayOfDOMNodes[i]);
> +    if (newNode) {

It's completely possible that this is just my unfamiliarity with editor/ talking, but this loop seems somewhat suspect to me: if anything in |outArrayOfDOMNodes| *isn't* an nsINode, then |outArrayOfNodes| is going to be smaller than |outArrayOfDOMNodes|.  And that's going to cause problems, because presumably |aIndex| initially refers to an index in |outArrayOfDOMNodes|, and it might not be a valid index into |outArrayOfNodes|, right?

Is the right thing to MOZ_ASSERT(newNode) here, MOZ_ASSERT about lengths of each list, MOZ_ASSERT that *aIndex is appropriately in bounds, or something else?

::: editor/libeditor/nsHTMLEditRules.h
@@ +200,5 @@
>    nsresult GetFormatString(nsIDOMNode *aNode, nsAString &outFormat);
> +  enum class Lists { no, yes };
> +  enum class Tables { no, yes };
> +  nsresult GetInnerContent(nsIDOMNode* aNode,
> +                           nsCOMArray<nsIDOMNode>& outArrayOfNodes,

Apologies for stepping on local coding style here, but shouldn't this really be aOutArrayOfNodes (and so on in other declarations and definitions introduced by this patch)?

@@ +206,5 @@
> +                           bool aTables = true);
> +  void GetInnerContent(nsINode& aNode,
> +                       nsTArray<nsCOMPtr<nsINode>>& outArrayOfNodes,
> +                       int32_t* aIndex, Lists aLists = Lists::yes,
> +                       Tables aTables = Tables::yes);

Am I correct in assuming that converting existing callers of GetInnerContent is either a) difficult; or b) coming in some distant-future patch?
Comment on attachment 8592756 [details] [diff] [review]
Part 2 - Make nsDOMIterator infallible

Review of attachment 8592756 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsEditorUtils.cpp
@@ +77,5 @@
> +nsDOMIterator::nsDOMIterator(nsIDOMNode& aNode)
> +{
> +  mIter = NS_NewContentIterator();
> +  nsCOMPtr<nsINode> node = do_QueryInterface(&aNode);
> +  NS_ENSURE_TRUE(node, );

NS_ENSURE_TRUE_VOID(node) is a little more idiomatic, I think, though I guess none of our compilers complain about empty macro arguments anymore?
Attachment #8592756 - Flags: review?(nfroyd) → review+
Comment on attachment 8592759 [details] [diff] [review]
Part 3 - Clean up nsHTMLEditRules::BustUpInlinesAtBRs

Review of attachment 8592759 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +6338,4 @@
>    iter.AppendList(functor, arrayOfBreaks);
> +
> +  // If there aren't any breaks, just put inNode itself in the array
> +  if (!arrayOfBreaks.Length()) {

Nit: arrayOfBreaks.IsEmpty()

::: editor/libeditor/nsHTMLEditRules.h
@@ +295,5 @@
>    nsresult GetParagraphFormatNodes(nsCOMArray<nsIDOMNode>& outArrayOfNodes, bool aDontTouchContent=false);
>    nsresult LookInsideDivBQandList(nsCOMArray<nsIDOMNode>& aNodeArray);
>    nsresult BustUpInlinesAtRangeEndpoints(nsRangeStore &inRange);
> +  nsresult BustUpInlinesAtBRs(nsINode& aNode,
> +                              nsTArray<nsCOMPtr<nsINode>>& outArrayOfNodes);

Coding style nit: aOutArrayOfNodes?
Attachment #8592759 - Flags: review?(nfroyd) → review+
Attachment #8592762 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #13)
> I guess you really need this in a later patch?

Hmm, yeah.  Doesn't matter, though.

> It's completely possible that this is just my unfamiliarity with editor/
> talking, but this loop seems somewhat suspect to me: if anything in
> |outArrayOfDOMNodes| *isn't* an nsINode, then |outArrayOfNodes| is going to
> be smaller than |outArrayOfDOMNodes|.  And that's going to cause problems,
> because presumably |aIndex| initially refers to an index in
> |outArrayOfDOMNodes|, and it might not be a valid index into
> |outArrayOfNodes|, right?
> 
> Is the right thing to MOZ_ASSERT(newNode) here, MOZ_ASSERT about lengths of
> each list, MOZ_ASSERT that *aIndex is appropriately in bounds, or something
> else?

Ehsan has said that shipping code has to not crash if it runs into an nsIDOMNode that isn't an nsINode, but it can do whatever else it feels like.  Maybe asserting is bad too.  So you're right, this is bad, it could cause asserts.  But I'm pretty sure he's said it doesn't matter as long as the code is removed in a subsequent patch, in which case this is fine.  I think the situation can only arise in extensions, maybe.  All nsIDOMNodes generated by mozilla-central code are also nsINodes.

> ::: editor/libeditor/nsHTMLEditRules.h
> @@ +200,5 @@
> >    nsresult GetFormatString(nsIDOMNode *aNode, nsAString &outFormat);
> > +  enum class Lists { no, yes };
> > +  enum class Tables { no, yes };
> > +  nsresult GetInnerContent(nsIDOMNode* aNode,
> > +                           nsCOMArray<nsIDOMNode>& outArrayOfNodes,
> 
> Apologies for stepping on local coding style here, but shouldn't this really
> be aOutArrayOfNodes (and so on in other declarations and definitions
> introduced by this patch)?

What you say makes more sense, but I think editor exclusively uses "out" instead of "aOut".  (I'm not at my work computer, so I didn't check.)

> Am I correct in assuming that converting existing callers of GetInnerContent
> is either a) difficult; or b) coming in some distant-future patch?

Patch 9 removes the nsCOMArray variant of GetInnerContent.

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #14)
> NS_ENSURE_TRUE_VOID(node) is a little more idiomatic, I think, though I
> guess none of our compilers complain about empty macro arguments anymore?

I wasn't aware it existed.  Given that NS_ENSURE_TRUE(node, ) works, I prefer it, because it cuts down on the number of distinct macros that you need to know about.
Ehsan, do I remember correctly that you said it's okay for a patch to assert or crash on nsIDOMNode-that's-not-nsINode as long as the code is removed in a subsequent patch before release?
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor from comment #17)
> Ehsan, do I remember correctly that you said it's okay for a patch to assert
> or crash on nsIDOMNode-that's-not-nsINode as long as the code is removed in
> a subsequent patch before release?

Yeah, that should be fine.
Flags: needinfo?(ehsan)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #13)
> Apologies for stepping on local coding style here, but shouldn't this really
> be aOutArrayOfNodes (and so on in other declarations and definitions
> introduced by this patch)?

It seems there's no really definite single coding style, so I'll go ahead and change these to aOut.
Comment on attachment 8592753 [details] [diff] [review]
Part 1 - Clean up nsHTMLEditRules::GetInnerContent

Review of attachment 8592753 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, given that the problematic QI code is apparently disappearing (in a patch yet to be posted?).
Attachment #8592753 - Flags: review?(nfroyd) → review+
Comment on attachment 8592766 [details] [diff] [review]
Part 5 - Clean up nsHTMLEditRules::GetNodesForOperation

Review of attachment 8592766 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with aOutArrayOfNodes
Attachment #8592766 - Flags: review?(nfroyd) → review+
Comment on attachment 8592767 [details] [diff] [review]
Part 6 - Clean up nsHTMLEditRules::LookInsideDivBQandList

Review of attachment 8592767 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with aOutArrayOfNodes
Attachment #8592767 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #20)
> r=me, given that the problematic QI code is apparently disappearing (in a
> patch yet to be posted?).

That entire overload of GetInnerContent is removed in part 9, attached to this bug.
Comment on attachment 8592769 [details] [diff] [review]
Part 7 - Clean up nsHTMLEditRules::PromoteRange

Review of attachment 8592769 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +6025,5 @@
>                                      bool aEntireList,
>                                      bool aDontTouchContent)
>  {
>    nsresult res = NS_OK;
> +

Uber nit: you didn't make any substantial changes to this function, but you did make some whitespace changes.  It'd be better if the whitespace was left alone for an editor-wide cleanup or something.  (I mention this only because other patches have been pretty consistent about not touching whitespace unless the lines with the whitespace have been changed themselves.)
Attachment #8592769 - Flags: review?(nfroyd) → review+
Attachment #8592772 - Flags: review?(nfroyd) → review+
Comment on attachment 8592775 [details] [diff] [review]
Part 9 - Clean up nsHTMLEditRules::GetNodesFromSelection

Review of attachment 8592775 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +965,5 @@
>  
>    // contruct a list of nodes to act on.
> +  nsTArray<nsCOMPtr<nsINode>> arrayOfNodes;
> +  nsresult res = GetNodesFromSelection(*selection, EditAction::indent,
> +                                       arrayOfNodes, TouchContent::no);

true meant "no"...hooray for using enums instead.

@@ +5161,5 @@
>  // GetInnerContent: aList and aTbl allow the caller to specify what kind of
>  //                  content to "look inside".  If aTbl is true, look inside any
>  //                  table content, and insert the inner content into the
>  //                  supplied issupportsarray at offset aIndex.  Similarly with
>  //                  aList and list content.  aIndex is updated to point past

This comment wants updating to reflect the new types and names of aLists and aTables.

@@ +6028,3 @@
>    {
>      // We don't like other people messing with our selection!
>      NS_ENSURE_STATE(mHTMLEditor);

Followup to remove the NS_ENSURE_STATE(mHTMLEditor) in this function and replace it with the:

  NS_ENSURE_TRUE(mHTMLEditor, );
  nsCOMPtr<> kungFuDeathGrip(mHTMLEditor);

idiom you've been using?  Or fold it into part one, I suppose.
Attachment #8592775 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #24)
> Uber nit: you didn't make any substantial changes to this function, but you
> did make some whitespace changes.  It'd be better if the whitespace was left
> alone for an editor-wide cleanup or something.  (I mention this only because
> other patches have been pretty consistent about not touching whitespace
> unless the lines with the whitespace have been changed themselves.)

The function is cleaned up anyway in bug 1153629 part 1, which will be pushed at the same time as this, so I think I won't spend the time moving the whitespace changes from one patch to another . . .

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #25)
> Followup to remove the NS_ENSURE_STATE(mHTMLEditor) in this function and
> replace it with the:
> 
>   NS_ENSURE_TRUE(mHTMLEditor, );
>   nsCOMPtr<> kungFuDeathGrip(mHTMLEditor);
> 
> idiom you've been using?  Or fold it into part one, I suppose.

I now do this routinely when I clean up the function, and I don't want to have to follow a follow-up for every function in editor I haven't cleaned up.  :)  But as noted, this was already done in bug 1153629 part 1.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #22)
> Comment on attachment 8592767 [details] [diff] [review]
> Part 6 - Clean up nsHTMLEditRules::LookInsideDivBQandList
> 
> Review of attachment 8592767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with aOutArrayOfNodes

This isn't really relevant to LookInsideDivBQandList, since it's the parameter of GetListActionNodes, not LookInsideDivBQandList.  But I'll fold it into the cleanup of GetListActionNodes in the other bug.
Hi, this changes got in backout in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=329dd852c06b since one of this changes caused memory leaks in linux/OS X m2,m4, M-e10s m4 and Crashtests like:

https://treeherder.mozilla.org/logviewer.html#?job_id=9178717&repo=mozilla-inbound

06:21:14 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | default process: 1483211 bytes leaked (AsyncLatencyLogger, AsyncStatement, AtomImpl, BodyRule, CSSStyleSheet, ...)
06:21:14 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, plugin process 1044
06:21:14 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
06:21:14 INFO - Per-Inst Leaked Total Rem
06:21:14 INFO - 0 TOTAL 57 0 301 0
06:21:14 INFO - nsTraceRefcnt::DumpStatistics: 26 entries
Flags: needinfo?(ayg)
Almost certain this is caused by bug 1157848.  Will reland with that if it passes the try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d3eb6ab4913
Flags: needinfo?(ayg)
Depends on: 1165982
You need to log in before you can comment on or make changes to this bug.