1149163 - Begin removing nsCOMArray from nsHTMLEditRules

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This time I'll get a green try run before attaching patches.  Just filing for the bug number.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 - Clean up nsHTMLEditRules::GetInnerContent

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.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 - Make nsDOMIterator infallible

For the MOZ_ASSERT(NS_SUCCEEDED(res))s from nsIContentIterator::Init, code inspection seems to indicate no actual failure path.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 - Clean up nsHTMLEditRules::BustUpInlinesAtBRs

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4 - Allow use of temporary nsBoolDomIterFunctor

Is this the right way to do this?

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 5 - Clean up nsHTMLEditRules::GetNodesForOperation

Do we want a const version of GetAsDOMNode, or is const nsINode* uncommon enough that const_cast is good enough?

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 6 - Clean up nsHTMLEditRules::LookInsideDivBQandList

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 7 - Clean up nsHTMLEditRules::PromoteRange

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 8 - Clean up nsHTMLEditRules::GetPromotedRanges

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 9 - Clean up nsHTMLEditRules::GetNodesFromSelection

I have a green try run, but I adjusted some of the patches in the course of submitting, so I'll get another one.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8bc8694cf2

Comment 12

[(no longer active)]

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!

Comment 13

[Nathan Froyd [:froydnj]]

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 14

[Nathan Froyd [:froydnj]]

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?

Comment 15

[Nathan Froyd [:froydnj]]

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?

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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?

Comment 18

[(no longer active)]

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

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

[Nathan Froyd [:froydnj]]

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?).

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8592766 [details] [diff] [review]
Part 5 - Clean up nsHTMLEditRules::GetNodesForOperation

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

r=me with aOutArrayOfNodes

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8592767 [details] [diff] [review]
Part 6 - Clean up nsHTMLEditRules::LookInsideDivBQandList

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

r=me with aOutArrayOfNodes

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

[Nathan Froyd [:froydnj]]

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

Comment 25

[Nathan Froyd [:froydnj]]

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.

Comment 26

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4392cf02f1c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc75a87a2b7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2817e4601371
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf082213f82
https://hg.mozilla.org/integration/mozilla-inbound/rev/152ee688999b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e36d2f251276
https://hg.mozilla.org/integration/mozilla-inbound/rev/a31a87b6dfb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a8ecc0301d
https://hg.mozilla.org/integration/mozilla-inbound/rev/641439af501f

Comment 29

[Carsten Book [:Tomcat]]

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

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b927da3c4832
https://hg.mozilla.org/integration/mozilla-inbound/rev/660729ebfe51
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe0ae724d95
https://hg.mozilla.org/integration/mozilla-inbound/rev/08972f5bba75
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced7f5966cd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c91c9f72c545
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9315e45f4e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/401c3e1b1c6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a861d44d6455

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b927da3c4832
https://hg.mozilla.org/mozilla-central/rev/660729ebfe51
https://hg.mozilla.org/mozilla-central/rev/fbe0ae724d95
https://hg.mozilla.org/mozilla-central/rev/08972f5bba75
https://hg.mozilla.org/mozilla-central/rev/ced7f5966cd5
https://hg.mozilla.org/mozilla-central/rev/c91c9f72c545
https://hg.mozilla.org/mozilla-central/rev/a9315e45f4e9
https://hg.mozilla.org/mozilla-central/rev/401c3e1b1c6f
https://hg.mozilla.org/mozilla-central/rev/a861d44d6455