Closed Bug 1154701 Opened 10 years ago Closed 10 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.akhgari
: review+
Details | Diff | Splinter Review
60.86 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
20.78 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
3.71 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
5.47 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
9.80 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
7.13 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
10.40 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
11.57 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
15.26 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
3.79 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
5.34 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
13.90 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
9.38 KB, patch
ehsan.akhgari
: 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.
Depends on: 1157848
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: