1358979 - nsDocumentEncoder with RequiresReinitAfterOutput should reset some strong pointers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Makoto Kato [:m_kato]]

Follow up bug 1352882.  nsDocumentEncoder with RequiresReinitAfterOutput should reset some strong pointers.

Comment 1

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

I think that it should release *all* strong pointers instead of some of them which causes leaking memory *now* since it can be regressed if somebody changes the implementation which is referred by nsDocumentEncoder strongly unless we can detect such regression with automated tests.

Comment 2

[Makoto Kato [:m_kato]]

If releasing mSerializer, it causes performance issue again.  mSerializer isn't CC class, I will add it to NS_IMPL_CYCLE_COLLECTION

Comment 3

[Makoto Kato [:m_kato]]

(When I profile code before landing bug 1452882, memory allocation for content serializer spends a lot of time)

Comment 4

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f4ccecbfcb8c9671cd09bc554e257ab17207dc6

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1358979 - nsDocumentEncoder with RequiresReinitAfterOutput should reset some strong pointers.

We should release strong pointers into nsDocumentEncoder with RequiresReinitAfterOutput option to avoid holding dom node.

Also, mStream and mUnicodeEncoder will release into EncodeToStream, so it is unnecessary to clear it.

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

Comment 6

[Makoto Kato [:m_kato]]

Attachment: Bug 1358979 - nsPlainTextSerializer should be cycle collection.

nsPlainTextSerializer holds Element, so it should use cycle collection class.

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

Comment 7

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

FWIW, currently I think this is not the cause for those leaks.
https://bugzilla.mozilla.org/show_bug.cgi?id=1357872#c17

Comment 8

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

Comment on attachment 8860889 [details]
Bug 1358979 - nsDocumentEncoder with RequiresReinitAfterOutput should reset some strong pointers.

https://reviewboard.mozilla.org/r/132892/#review135678

Comment 9

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

Comment on attachment 8860890 [details]
Bug 1358979 - nsPlainTextSerializer should be cycle collection.

https://reviewboard.mozilla.org/r/132894/#review135682

Comment 10

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

Unfortunately this doesn't seem to help with Bug 1357872. No idea yet what caused that bug.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83f12eb017b6
nsDocumentEncoder with RequiresReinitAfterOutput should reset some strong pointers. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a01a94c69531
nsPlainTextSerializer should be cycle collection. r=smaug

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83f12eb017b6
https://hg.mozilla.org/mozilla-central/rev/a01a94c69531