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)
Core
DOM: Core & HTML
Tracking
()
NEW
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.
Comment 1•7 years ago
|
||
Or we shouldn't have multiple range objects. Are the range objects from editor?
Comment 2•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 3•7 years ago
|
||
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.
Comment 4•7 years ago
|
||
> 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.)
Reporter | ||
Comment 5•7 years ago
|
||
(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.
Updated•7 years ago
|
Priority: -- → P2
Comment 6•6 years ago
|
||
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
Reporter | ||
Comment 7•4 years ago
|
||
Resetting assignee which I don't work on in this several months.
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•