Closed
Bug 1154701
Opened 10 years ago
Closed 10 years ago
Finish (almost) killing nsCOMArray from editor
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
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-
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8593901 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8593903 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8593904 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8593904 -
Attachment is obsolete: true
Attachment #8593904 -
Flags: review?(ehsan)
Attachment #8593906 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8593907 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8593907 -
Attachment is obsolete: true
Attachment #8593907 -
Flags: review?(ehsan)
Attachment #8593908 -
Flags: review?(ehsan)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8593909 -
Flags: review?(ehsan)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8593912 -
Flags: review?(ehsan)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8593916 -
Flags: review?(ehsan)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8593917 -
Flags: review?(ehsan)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8593919 -
Flags: review?(ehsan)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8593923 -
Flags: review?(ehsan)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8593925 -
Flags: review?(ehsan)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8593927 -
Flags: review?(ehsan)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8593931 -
Flags: review?(ehsan)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8593934 -
Flags: review?(ehsan)
Comment 17•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8593903 -
Flags: review?(ehsan) → review+
Comment 18•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8593908 -
Flags: review?(ehsan) → review+
Comment 19•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8593912 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8593916 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8593917 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8593919 -
Flags: review?(ehsan) → review+
Comment 20•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8593925 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8593927 -
Flags: review?(ehsan) → review+
Comment 21•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8593934 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(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.
Assignee | ||
Comment 23•10 years ago
|
||
(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.
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/81825eff1936
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a83215797f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad34b482c25
https://hg.mozilla.org/integration/mozilla-inbound/rev/31281738003d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab63df2b8eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d41b284d92
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd58656d297
https://hg.mozilla.org/integration/mozilla-inbound/rev/07620ff1a21a
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f9ae64772d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd74af965fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a57f36dc00b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9801778d9d5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd3566e571c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e64fa8717641
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
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.
Assignee | ||
Comment 27•10 years ago
|
||
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)
Comment 28•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/704a534031c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d852b7730ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/6458a7f720df
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6d373f7ce6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9818d632cca
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e533274a62
https://hg.mozilla.org/integration/mozilla-inbound/rev/168424b50f5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4733c09957da
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7081ca288c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/568c7eff6b66
https://hg.mozilla.org/integration/mozilla-inbound/rev/dea5a5323d5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b7fb80d5f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/aed4b9f5f2d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/afef0f347312
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/704a534031c8
https://hg.mozilla.org/mozilla-central/rev/4d852b7730ba
https://hg.mozilla.org/mozilla-central/rev/6458a7f720df
https://hg.mozilla.org/mozilla-central/rev/ed6d373f7ce6
https://hg.mozilla.org/mozilla-central/rev/c9818d632cca
https://hg.mozilla.org/mozilla-central/rev/c0e533274a62
https://hg.mozilla.org/mozilla-central/rev/168424b50f5f
https://hg.mozilla.org/mozilla-central/rev/4733c09957da
https://hg.mozilla.org/mozilla-central/rev/d7081ca288c7
https://hg.mozilla.org/mozilla-central/rev/568c7eff6b66
https://hg.mozilla.org/mozilla-central/rev/dea5a5323d5c
https://hg.mozilla.org/mozilla-central/rev/c6b7fb80d5f1
https://hg.mozilla.org/mozilla-central/rev/aed4b9f5f2d9
https://hg.mozilla.org/mozilla-central/rev/afef0f347312
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•