TextEditor should cache nsIDocumentEncoder it uses for OutputToString/Stream

RESOLVED FIXED in Firefox 55

Status

()

Core
Editor
P2
normal
RESOLVED FIXED
2 months ago
29 days ago

People

(Reporter: smaug, Assigned: m_kato)

Tracking

(Blocks: 1 bug)

36 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Comment 1

2 months ago
Another option is to cache some instance of nsIDocumentEncoder globally and re-initialize it and use it when needed.
(Assignee)

Updated

2 months ago
Assignee: nobody → m_kato
Priority: -- → P2
Whiteboard: [qf]
Comment hidden (mozreview-request)
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-
Whiteboard: [qf] → [qf:p1]
(Assignee)

Updated

a month ago
Attachment #8856393 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I'll review it after smaug gives r+ to "Part 1".
(Reporter)

Comment 8

a month 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

a month 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 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

a month 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

a month 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

a month 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

a month 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
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Comment 15

a month ago
Could this have caused bug 1357872?
Flags: needinfo?(m_kato)
(Reporter)

Comment 16

a month 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

a month ago
Humm, selection, node and parent node isn't set to nullptr, does this occur?  I will file a new bug to unset it.
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)

Updated

a month ago
Depends on: 1358979
(Assignee)

Comment 19

a month ago
And, nsPlainTextSerializer holds Element, but it doesn't have NS_IMPL_CYCLE_COLLECTION defines.
(Reporter)

Comment 20

a month 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

29 days ago
Flags: needinfo?(m_kato)
You need to log in before you can comment on or make changes to this bug.