1352882 - TextEditor should cache nsIDocumentEncoder it uses for OutputToString/Stream

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Another option is to cache some instance of nsIDocumentEncoder globally and re-initialize it and use it when needed.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1352882 - Cache nsIDocumentEncoder into TextEditor.

All editor code gets nsIDocumentEncoder from TextEditor::GetAndInitDocEncoder(), so we can have a cache into TextEditor, then return cached object.

Review commit: https://reviewboard.mozilla.org/r/128324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128324/

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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!)

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 1352882 - Part 1. Add releasing nsIDcoument option to recycle nsIDocumentEncoder.

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.

Review commit: https://reviewboard.mozilla.org/r/129818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129818/

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1352882 - Part 2. Add test for ReleaseDocumentAfterOutput.

Review commit: https://reviewboard.mozilla.org/r/129820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129820/

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Bug 1352882 - Part 3. Cache nsIDocumentEncoder into TextEditor.

All editor code gets nsIDocumentEncoder from TextEditor::GetAndInitDocEncoder(), so we can have a cache into TextEditor, then return cached object.

Review commit: https://reviewboard.mozilla.org/r/129822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129822/

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll review it after smaug gives r+ to "Part 1".

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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 ?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Makoto Kato [:m_kato]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/916cf393324e
https://hg.mozilla.org/mozilla-central/rev/20c5c8bf2357
https://hg.mozilla.org/mozilla-central/rev/23e9df36a42a

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Could this have caused bug 1357872?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I'm seeing the editor objects with mCachedDocumentEncoder set in CC graphs. That doesn't mean really anything, but is a bit suspicious.

Comment 17

[Makoto Kato [:m_kato]]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 19

[Makoto Kato [:m_kato]]

And, nsPlainTextSerializer holds Element, but it doesn't have NS_IMPL_CYCLE_COLLECTION defines.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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.