Open Bug 1370756 Opened 7 years ago Updated 2 years ago

Taking long time in nsRange::ContentAppended() while setting innerHTML

Categories

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

defect

Tracking

()

People

(Reporter: masayuki, Unassigned)

References

Details

The stack when nsRnage::ContentAppended() is called: >> xul.dll!nsRange::ContentAppended(nsIDocument * aDocument, nsIContent * aContainer, nsIContent * aFirstNewContent, int aNewIndexInContainer) Line 609 C++ > xul.dll!nsNodeUtils::ContentAppended(nsIContent * aContainer, nsIContent * aFirstNewContent, int aNewIndexInContainer) Line 167 C++ > xul.dll!nsHtml5TreeOperation::Append(nsIContent * aNode, nsIContent * aParent, nsHtml5DocumentBuilder * aBuilder) Line 186 C++ > xul.dll!nsHtml5TreeOperation::AppendText(const char16_t * aBuffer, unsigned int aLength, nsIContent * aParent, nsHtml5DocumentBuilder * aBuilder) Line 167 C++ > xul.dll!nsHtml5TreeBuilder::appendCharacters(void * aParent, char16_t * aBuffer, int aStart, int aLength) Line 567 C++ > xul.dll!nsHtml5TreeBuilder::flushCharacters() Line 4504 C++ > xul.dll!nsHtml5TreeBuilder::endTag(nsHtml5ElementName * elementName) Line 2308 C++ > xul.dll!nsHtml5Tokenizer::emitCurrentTagToken(bool selfClosing, int pos) Line 327 C++ > xul.dll!nsHtml5Tokenizer::stateLoop<nsHtml5SilentPolicy>(int state, char16_t c, int pos, char16_t * buf, bool reconsume, int returnState, int endPos) Line 988 C++ > xul.dll!nsHtml5Tokenizer::tokenizeBuffer(nsHtml5UTF16Buffer * buffer) Line 445 C++ > xul.dll!nsHtml5StringParser::Tokenize(const nsAString & aSourceBuffer, nsIDocument * aDocument, bool aScriptingEnabledForNoscriptParsing) Line 115 C++ > xul.dll!nsHtml5StringParser::ParseFragment(const nsAString & aSourceBuffer, nsIContent * aTargetNode, nsIAtom * aContextLocalName, int aContextNamespace, bool aQuirks, bool aPreventScriptExecution) Line 62 C++ > xul.dll!nsContentUtils::ParseFragmentHTML(const nsAString & aSourceBuffer, nsIContent * aTargetNode, nsIAtom * aContextLocalName, int aContextNamespace, bool aQuirks, bool aPreventScriptExecution) Line 4984 C++ > xul.dll!mozilla::dom::FragmentOrElement::SetInnerHTMLInternal(const nsAString & aInnerHTML, mozilla::ErrorResult & aError) Line 2361 C++ Multiple nsRange instances handles same node. I think that nsNodeUtils should do it once.
Or we shouldn't have multiple range objects. Are the range objects from editor?
Or perhaps Range code could have some cache, and if there are range objects observing same node + offset, calculate value only once. nsNodeUtils just passes nsIMutationObserver notification, which is a generic API, so adding some range specific there would feel odd.
I'm now thinking that if Selection observes mutation, it can reduce the nsRange's jobs. Additionally, it could allow to make Selection store simpler range struct or class.
> I'm now thinking that if Selection observes mutation, it can reduce the nsRange's jobs. But a Selection typically only has one nsRange, so I don't see what the improvement is for the common case. Having multiple ranges per Selection is very rare. And pages with dozens of <input>s are also quite rare, so I'm not sure how much a typical web page will benefit from optimizations in this area. That said, we could centralize updating nsRanges by adding a class specifically to do that task, and remove the Observer code from nsRange. Doing that centrally (per document) probably allows some optimizations to be made. (Again, this bug would benefit from a more detailed description of the problem/use case, to make it easier discuss possible solutions.)
(In reply to Mats Palmgren (:mats) from comment # > > I'm now thinking that if Selection observes mutation, it can reduce the nsRange's jobs. > > But a Selection typically only has one nsRange, so I don't see what > the improvement is for the common case. Having multiple ranges per > Selection is very rare. Looks like that nsRange::SetSelection() is called for any type of Selection: https://searchfox.org/mozilla-central/rev/b2d072aa5847996b8276bd8d7b150e0ea6bf6283/layout/generic/nsSelection.cpp#3952,3964,4004,4051 If the selection is SelectionType::eNormal, yes it is. However, I worry about SelectionType::eSpellCheck and SelectionType::eFind (I'm not sure about SelectionType::eAccessibility). IIRC, there can be about 100 or 200 nsRange instances. If nsRange has non-nullptr mSelection, nsRange::ContentAppended() does a lot: https://searchfox.org/mozilla-central/rev/b2d072aa5847996b8276bd8d7b150e0ea6bf6283/dom/base/nsRange.cpp#608-618 > That said, we could centralize updating nsRanges by adding a class > specifically to do that task, and remove the Observer code from > nsRange. Doing that centrally (per document) probably allows some > optimizations to be made. Okay, do you think where is a good place to add mutation observer? Currently, nsRange adds itself to mutation observer of root node of subtree if it's in a subtree and mMaySpanAnonymousSubtrees is false or root node (document node? root element? not sure) of the parent document, otherwise. > (Again, this bug would benefit from a more detailed description of > the problem/use case, to make it easier discuss possible solutions.) I found this bug when I investigate bug 1370754 which is inserting a lot of elements to focused editor with innerHTML.
Priority: -- → P2
Moving to p3 because no activity for at least 1 year(s). See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3

Resetting assignee which I don't work on in this several months.

Assignee: masayuki → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.