Closed
Bug 1352882
Opened 8 years ago
Closed 8 years ago
TextEditor should cache nsIDocumentEncoder it uses for OutputToString/Stream
Categories
(Core :: DOM: Editor, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: smaug, Assigned: m_kato)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
No description provided.
Reporter | ||
Comment 1•8 years ago
|
||
Another option is to cache some instance of nsIDocumentEncoder globally and re-initialize it and use it when needed.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → m_kato
Priority: -- → P2
Updated•8 years ago
|
Whiteboard: [qf]
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8856393 [details] Bug 1352882 - Cache nsIDocumentEncoder into TextEditor. https://reviewboard.mozilla.org/r/128324/#review130844 ::: editor/libeditor/TextEditor.cpp:1194 (Diff revision 1) > nsCOMPtr<nsIDOMDocument> domDoc = do_QueryReferent(mDocWeak); > NS_ASSERTION(domDoc, "Need a document"); > > - rv = docEncoder->Init(domDoc, aFormatType, aFlags); > + nsresult rv = docEncoder->Init(domDoc, aFormatType, aFlags); Basically, I don't disagree with this patch. However, this may have a memory leak bug. Editor doesn't grab document, instead, it uses weak pointer. However, nsDocumentEncoder does it. So, caching document encoder with strong pointer means same as there is |nsCOMPtr<nsIDOMDocument> mDocument|. So, I think that you need to add a method to release objects and the callers need to use it. (If my review is wrong, we can use strong pointer in editor to store document. It must make editor simpler!)
Attachment #8856393 -
Flags: review?(masayuki) → review-
Updated•8 years ago
|
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Updated•8 years ago
|
Attachment #8856393 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8857810 [details] Bug 1352882 - Part 1. Add releasing nsIDcoument option to recycle nsIDocumentEncoder. https://reviewboard.mozilla.org/r/129818/#review132498 ::: commit-message-aca6b:1 (Diff revision 1) > +Bug 1352882 - Part 1. Add releasing nsIDcoument option to recycle nsIDocumentEncoder. r?smaug nsIDocument. ::: commit-message-aca6b:3 (Diff revision 1) > +Bug 1352882 - Part 1. Add releasing nsIDcoument option to recycle nsIDocumentEncoder. r?smaug > + > +Editor uses weak reference for nsIDocument. But nsDocumentEncoder uses strong reference for it. So to recycle it, editor wants the option that it release nsIDocument into nsDocumentEncoder after EncodeTo* is called. This comment is unclear. What 'it' refers in different sentences? I read the sentences so that "to recycle it" refers to a document object, but I think you mean document encoder. ::: dom/base/nsIDocumentEncoder.idl:251 (Diff revision 1) > * not be broken just as "normal" longs strings aren't broken. > */ > const unsigned long OutputDisallowLineBreaking = (1 << 27); > > /** > + * Release reference of nsIDocument after using encodeTo* method to recycle A bit hackish, but probably fine. But, could we call the flag something else. Maybe RequiresReinitAfterOutput ?
Attachment #8857810 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8857811 [details] Bug 1352882 - Part 2. Add test for ReleaseDocumentAfterOutput. https://reviewboard.mozilla.org/r/129820/#review132500 with renaming everywhere, r+ ::: commit-message-13734:1 (Diff revision 1) > +Bug 1352882 - Part 2. Add test for ReleaseDocumentAfterOutput. r?smaug So, RequiresReinitAfterOutput here ::: dom/base/test/test_encodeToStringWithReleaseDocument.html:19 (Diff revision 1) > + const Cc = SpecialPowers.Cc; > + > + // Create a plaintext encoder without flags. > + var encoder = Cc["@mozilla.org/layout/documentEncoder;1?type=text/plain"] > + .createInstance(de); > + encoder.init(document, "text/plain", encoder.ReleaseDocumentAfterOutput); RequiresReinitAfterOutput and elsewhere. ::: dom/base/test/test_encodeToStringWithReleaseDocument.html:23 (Diff revision 1) > + .createInstance(de); > + encoder.init(document, "text/plain", encoder.ReleaseDocumentAfterOutput); > + return encoder; > + } > + > + function testPlaintextSerializerWithReleaseDocumentAfterOutput() { RequiresReinitAfterOutput
Attachment #8857811 -
Flags: review?(bugs) → review+
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8857812 [details] Bug 1352882 - Part 3. Cache nsIDocumentEncoder into TextEditor. https://reviewboard.mozilla.org/r/129822/#review132568 Looks hacky, but I have no better idea.
Attachment #8857812 -
Flags: review?(masayuki) → review+
Reporter | ||
Comment 11•8 years ago
|
||
One thing I was wondering at some point is that could we somehow use .innerText here? But didn't look at at all whether the result would be totally different.
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #11) > One thing I was wondering at some point is that could we somehow use > .innerText here? > But didn't look at at all whether the result would be totally different. Since nsGenericHTMLElement::GetInnerText uses Element::GetPrimaryFrame(FlushType::Layout) and nsRange::GetInnerTextNoFlush, so this is no help to improve getting innerText.
Comment 13•8 years ago
|
||
Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/916cf393324e Part 1. Add releasing nsIDocument option to recycle nsIDocumentEncoder. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/20c5c8bf2357 Part 2. Add test for RequiresReinitAfterOutput. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/23e9df36a42a Part 3. Cache nsIDocumentEncoder into TextEditor. r=masayuki
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/916cf393324e https://hg.mozilla.org/mozilla-central/rev/20c5c8bf2357 https://hg.mozilla.org/mozilla-central/rev/23e9df36a42a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Reporter | ||
Comment 16•8 years ago
|
||
I'm seeing the editor objects with mCachedDocumentEncoder set in CC graphs. That doesn't mean really anything, but is a bit suspicious.
Assignee | ||
Comment 17•8 years ago
|
||
Humm, selection, node and parent node isn't set to nullptr, does this occur? I will file a new bug to unset it.
Comment 18•8 years ago
|
||
TextEditor::GetAndInitDocEncoder() calls nsDocumentEncoder::SetSelection() or nsDocumentEncoder::SetNativeContainerNode(). The former caches Selection with mSelection and the latter caches nsINode with mNode. Grabbing them from editor may cause leaking a lot of objects. AutoReleaseDocumentIfNeeded perhaps needs to release them too at least. Additionally, nsDocumentEncoder::EncodeToStringWithMaxLength() caches nsIContentSerializer with mSerializer and nsINode instances with mCommonAncestors. The former caches some nsINode instances so, these also may cause leaking memory. I bet that nsDocumentEncoder should have |Clear()| and it should release any strong pointers.
Assignee | ||
Comment 19•8 years ago
|
||
And, nsPlainTextSerializer holds Element, but it doesn't have NS_IMPL_CYCLE_COLLECTION defines.
Reporter | ||
Comment 20•8 years ago
|
||
Aha, that looks promising. I'm still not sure whether this caused bug 1357872, since I don't know how to reproduce (the leak just happens occasionally). I've been trying various builds between 04-17 and 04-18 in order to find the actual cause. But anyhow, if there are possible leaks, those should be fixed ASAP.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(m_kato)
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•