Closed Bug 1154701 Opened 6 years ago Closed 6 years ago

Finish (almost) killing nsCOMArray from editor

Categories

(Core :: DOM: Editor, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(14 files, 2 obsolete files)

16.01 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
60.86 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
20.78 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
3.71 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
5.47 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
9.80 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
7.13 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
10.40 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
11.57 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
15.26 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
3.79 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
5.34 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
13.90 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
9.38 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
There's one usage in nsTransactionList that I didn't want to touch because it looked scary.  It involves nsISupports***.
Flags: in-testsuite-
Attachment #8593907 - Attachment is obsolete: true
Attachment #8593907 - Flags: review?(ehsan)
Attachment #8593908 - Flags: review?(ehsan)
Comment on attachment 8593901 [details] [diff] [review]
Part 1 - Clean up nsHTMLEditor::CreateListOfNodesToPaste

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

::: editor/libeditor/nsHTMLDataTransfer.cpp
@@ +2200,5 @@
>    nsRefPtr<nsRange> docFragRange;
> +  nsresult rv = nsRange::CreateRange(aStartNode, aStartOffset,
> +                                     aEndNode, aEndOffset,
> +                                     getter_AddRefs(docFragRange));
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

Can we really assert success here?
Attachment #8593901 - Flags: review?(ehsan) → review+
Attachment #8593903 - Flags: review?(ehsan) → review+
Comment on attachment 8593906 [details] [diff] [review]
Part 3 - Clean up nsHTMLEditor::GetListAndTableParents, DiscoverPartialListsAndTables, ScanForListAndTableStructure, ReplaceOrphanedStructure

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

::: editor/libeditor/nsHTMLEditor.h
@@ +608,5 @@
> +  enum class StartOrEnd { start, end };
> +  void GetListAndTableParents(StartOrEnd aStartOrEnd,
> +                              nsTArray<mozilla::dom::OwningNonNull<nsINode>>& aNodeList,
> +                              nsTArray<mozilla::dom::OwningNonNull<mozilla::dom::Element>>& outArray);
> +  int32_t DiscoverPartialListsAndTables(nsTArray<mozilla::dom::OwningNonNull<nsINode>>& aPasteNodes,

Hmm, perhaps using a helper typedef for these arrays is a good idea.  This is getting really verbose.
Attachment #8593906 - Flags: review?(ehsan) → review+
Attachment #8593908 - Flags: review?(ehsan) → review+
Comment on attachment 8593909 [details] [diff] [review]
Part 5 - Switch nsHTMLEditor::objectResizeEventListeners to nsTArray

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

::: editor/libeditor/nsHTMLEditor.h
@@ +850,5 @@
>    nsCOMPtr<nsIDOMEventListener>  mMouseMotionListenerP;
>    nsCOMPtr<nsISelectionListener> mSelectionListenerP;
>    nsCOMPtr<nsIDOMEventListener>  mResizeEventListenerP;
>  
> +  nsTArray<mozilla::dom::OwningNonNull<nsIHTMLObjectResizeListener>> objectResizeEventListeners;

Nit: please rename to mObjectResizeEventListeners.
Attachment #8593909 - Flags: review?(ehsan) → review+
Attachment #8593912 - Flags: review?(ehsan) → review+
Attachment #8593916 - Flags: review?(ehsan) → review+
Attachment #8593917 - Flags: review?(ehsan) → review+
Attachment #8593919 - Flags: review?(ehsan) → review+
Comment on attachment 8593923 [details] [diff] [review]
Part 10 - Switch nsEditor::mActionListeners to nsTArray

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

::: editor/libeditor/nsEditor.h
@@ +6,5 @@
>  #ifndef __editor_h__
>  #define __editor_h__
>  
>  #include "mozilla/Assertions.h"         // for MOZ_ASSERT, etc.
> +#include "mozilla/dom/OwningNonNull.h"  // for OwningNonNull

Couldn't we forward-declare this intsead?
Attachment #8593923 - Flags: review?(ehsan) → review+
Attachment #8593925 - Flags: review?(ehsan) → review+
Attachment #8593927 - Flags: review?(ehsan) → review+
Comment on attachment 8593931 [details] [diff] [review]
Part 13 -Clean up nsHTMLEditor::SetCSSBackgroundColor

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

::: editor/libeditor/nsHTMLEditor.cpp
@@ +4509,5 @@
>    // Protect the edit rules object from dying
>    nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules);
>  
>    nsRefPtr<Selection> selection = GetSelection();
> +  NS_ENSURE_STATE(selection);

I wonder why the old code didn't null check this?
Attachment #8593931 - Flags: review?(ehsan) → review+
Attachment #8593934 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> Can we really assert success here?

It looks like it will only fail if the points are invalid.  It seems fine to assert the points are valid.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #20)
> Couldn't we forward-declare this intsead?

It doesn't look like it.  From the errors, I think one problem is the nsTArray destructor needs to know the size of its members.
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)
From one of the logs:
06:01:00     INFO -  [Child 1985] ###!!! ASSERTION: Destroying a currently-showing document: '!mIsShowing', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/base/nsDocument.cpp, line 1650
06:01:00     INFO -  [Child 1985] ###!!! ASSERTION: Document leaked to shutdown, then the observer service dropped its ref to us so we were able to go away.: '!mObservingAppThemeChanged', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/base/nsDocument.cpp, line 1656

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-debug/1429702090/mozilla-inbound_ubuntu64_vm-debug_test-mochitest-e10s-4-bm118-tests1-linux64-build180.txt.gz

Not sure if that's relevant or not.
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: 1158452
Depends on: 1166932
Depends on: 1165369
You need to log in before you can comment on or make changes to this bug.