Closed Bug 1892514 Opened 2 years ago Closed 2 years ago

Copy-paste of text from local text editor to Codepen demo is slow, but deleting that text is fast. Copy-paste of text from browser to Codepen demo is fast, but deleting that text is slow (STR in comment 8)

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
129 Branch
Tracking Flags:
Tracking Status
firefox129 --- fixed

People

(Reporter: mayankleoboy1, Assigned: masayuki)

References

(Depends on 2 open bugs,
URL
)

Details

Attachments

(3 files, 14 external files)

about:support
43.25 KB, text/plain
Details
d3.txt
569.17 KB, text/plain
Details
Standalone testcase.html
7.54 KB, text/html
Details

Go to https://codepen.io/bramus/pen/MWxLjEo
VERY IMPORTANT: Make sure that the demo-pane is to the left of the screen, and the code-pane is to the right.
Copy-paste the contents of the attached js text in the codepen demo textarea

Profile with demo-pane at left: https://share.firefox.dev/4aYF6Jx
Profile with demo-pane at bottom: https://share.firefox.dev/44e6Mbi
ER: Not so, maybe?

From https://bugzilla.mozilla.org/show_bug.cgi?id=1892216#c3 , :dholbert did not observe this much time to paste the text. If the demo-pane is to the bottom and the code-pen is at the top, then the profile is completely different (and matches what :dholbert saw).

Flags: needinfo?(masayuki)
Attached file about:support —
See Also: → 1892216
Attached file d3.txt —

Merged with comment 0

Summary: Pasting 500KB of text in a Codepen demo spends 7s in IME (and to my knowledge I dont use IME) → Pasting 500KB of text in a Codepen demo spends 7s in IME if the code-pane is to the left. If the code-pane is at the bottom, text is pasted almost instantanously.
Summary: Pasting 500KB of text in a Codepen demo spends 7s in IME if the code-pane is to the left. If the code-pane is at the bottom, text is pasted almost instantanously. → Pasting 500KB of text in a Codepen demo spends 7s in IME if the demo-pane is to the left. If the demo-pane is at the bottom, text is pasted almost instantanously.

I may have misdirected with the location of the code-pane. I dont repro the slow pasting at all now irrespective of the position of the code-pane. Make what you will of this.

I can reproduce.

  • Mayank's slow profile from comment 0, https://share.firefox.dev/4aYF6Jx , recorded with demo-pane at left [SLOW]
    7 second jank, with ~5.8 seconds spent in mozilla::IMEContentObserver::DocumentObserver::EndUpdate
    (The profiler shows 2905 samples, but that seems to be responsible for 5.8 seconds in the timeline view; we must be sampling every 2ms or something.)

  • My own profile, https://share.firefox.dev/3U7ukK1 , recorded with demo pane at bottom [Fine]
    Zero seconds spent in mozilla::IMEContentObserver::DocumentObserver::EndUpdate, though I do see 94ms in mozilla::IMEContentObserver::IMENotificationSender::SendTextChange

  • My own profile, https://share.firefox.dev/3xB0J47 , recorded with demo pane at left [SLOW]
    https://share.firefox.dev/3xB0J47
    8.4 second jank, with 5.8 seconds spent in mozilla::IMEContentObserver::DocumentObserver::EndUpdate

Note, the first time I profiled with the demo pane at the left, I didn't get much jank and didn't see DocumentObserver::EndUpdate in my profile; but the second time I did (that's the last profile that I linked above).

(In reply to Mayank Bansal from comment #4)

I may have misdirected with the location of the code-pane. I dont repro the slow pasting at all now irrespective of the position of the code-pane. Make what you will of this.

That's good to know -- per last paragraph in comment 5, I tried twice with the demo pane at the left, and I hit the issue in the second attempt but not the first.

(side note: it looks like Mayank's on Win11, and I'm on Ubuntu 22.04; given that we've both been able to intermittently repro, it seems like there's no platform-specific stuff here.)

OK, i think i get the STR. The key to repro this is to download the text to your local machine, open the text file in a local text editor, copy the text from the text editor and then paste it into the Codepen demo (irrespective of the position of the demo-pane). Pasting is slow, but then deleting the text is fast.

If on the other hand, you open the attached text file in the browser, copy the text, and then paste it the action will be fast (the text appears very fast but is usable only after a few seconds). But then deleting the text is slow.

Action : Paste the text, then Ctrl+A and Delete

Text copied from local text editor: https://share.firefox.dev/3UpM2d9 / https://share.firefox.dev/3U5dlYX
Text copied from opening the attachment in the browser: https://share.firefox.dev/3xJ4kgR / https://share.firefox.dev/3W5pDD8

Summary: Pasting 500KB of text in a Codepen demo spends 7s in IME if the demo-pane is to the left. If the demo-pane is at the bottom, text is pasted almost instantanously. → Copy-paste of text from local text editor to Codepen demo is slow, but deleting that text is fast. Copy-paste of text from browser to Codepen demo is fast, but deelting that text is slow
Summary: Copy-paste of text from local text editor to Codepen demo is slow, but deleting that text is fast. Copy-paste of text from browser to Codepen demo is fast, but deelting that text is slow → Copy-paste of text from local text editor to Codepen demo is slow, but deleting that text is fast. Copy-paste of text from browser to Codepen demo is fast, but deelting that text is slow (STR in comment 8)
See Also: → 1892589

Filed Bug 1892589 for the highlight API related jank in the second case ("Text copied from opening the attachment in the browser"). I wasn't completely able to reproduce the profiles mentioned above, however I could see a ~1s jank in Highlight::Add() on my machine which I could reduce to ~200ms. This syntax highlighting-with-highlight-api codepen is a gold mine, thank you Mayank and Daniel for raising this and providing this much input. This is much appreciated! :)

This is caused by what is discussed in bug 1844470. This is a good example to fix the issue. I'll take a look next week.

Depends on: 1844470
Flags: needinfo?(masayuki)

Hmm, I don't reproduce this bug when I copy the text from some test editors including vscode. I see only less than 30 <br> elements in at pasting the attached JS file. I wonder, the clipboard format may depend on text editor and OS. Mayank, do you use which OS and which text editor?

Flags: needinfo?(mayankleoboy1)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #11)

Hmm, I don't reproduce this bug when I copy the text from some test editors including vscode. I see only less than 30 <br> elements in at pasting the attached JS file. I wonder, the clipboard format may depend on text editor and OS. Mayank, do you use which OS and which text editor?

I use Windows11 (also see the about:support i have attached to this bug)
I reproduce this when using two different text-editors : Notepad (i.e. the builtin Windows11 notepad), and Notepad++.
Here is a profile of copying the text from Notepad : https://share.firefox.dev/4aJu4Iu

Could it be that I have some hidden IME service on my system (maybe due to some Windows11 or MS-Office setting?)

Edit: I tested on Chrome, and the behaviour is similar to Nightly - that is, slow paste when text is copied from a local text-editor, and fast paste when text is copied from the browser. The performance is worse than Nightly in both these cases.
This may imply that may be something inherently slow about Win11+local text-editors? But :dholbert could repro once on Linux too, so who knows whats really happening.

Flags: needinfo?(mayankleoboy1) → needinfo?(masayuki)

Thank you for the quick reply, but unfortunately I cannot reproduce this with them. Similar symptom occurs in jsfidle, but it spends in their JS file in most time. Therefore, it's not same as this bug.

Could it be that I have some hidden IME service on my system (maybe due to some Windows11 or MS-Office setting?)

We cannot prepare the content information for IME when they request the content to our parent process. Therefore, we always need to create a cache in the remote process and copy the cache to the parent when the content is changed. So, the cache could be unused in some environments, but we cannot consider the environment has IME or not since modern IME API may be used for other purposes from text input.

Edit: I tested on Chrome, and the behaviour is similar to Nightly - that is, slow paste when text is copied from a local text-editor, and fast paste when text is copied from the browser. The performance is worse than Nightly in both these cases.
This may imply that may be something inherently slow about Win11+local text-editors

I guess that it's slower if the clipboard format is text/plain.

Flags: needinfo?(masayuki)

Ah, okay, I misunderstood the STR. I tried to paste into the codepen's editor, not in the editor which is created by the script.

When I paste it, a ton of calls of ShouldBreakLineBefore for <br> elements. It seems that the bottleneck in the profile is not the direct cause of this bug. I think that we can skip caching content in some cases.

Some first call-stacks of IMEContentObserver::BeginDocumentUpdate().

#01: mozilla::dom::Document::BeginUpdate (M:\src\dom\base\Document.cpp:7977)
#02: mozilla::dom::Element::UnsetAttr (M:\src\dom\base\Element.cpp:2961)
#03: nsHTMLScrollFrame::SetScrollbarEnabled (M:\src\layout\generic\nsGfxScrollFrame.cpp:6745)
#04: nsHTMLScrollFrame::ReflowFinished (M:\src\layout\generic\nsGfxScrollFrame.cpp:6366)
#05: mozilla::PresShell::DidDoReflow (M:\src\layout\base\PresShell.cpp:9755)
#06: mozilla::PresShell::ProcessReflowCommands (M:\src\layout\base\PresShell.cpp:10162)
#07: mozilla::PresShell::DoFlushPendingNotifications (M:\src\layout\base\PresShell.cpp:4436)
#08: mozilla::dom::Document::FlushPendingNotifications (M:\src\dom\base\Document.cpp:10946)
#09: nsGenericHTMLElement::GetInnerText (M:\src\dom\html\nsGenericHTMLElement.cpp:3163)
#10: mozilla::dom::HTMLElement_Binding::get_innerText (M:\fx64-dbg\dom\bindings\HTMLElementBinding.cpp:378)

#01: mozilla::dom::Document::BeginUpdate (M:\src\dom\base\Document.cpp:7977)
#02: mozilla::dom::Element::UnsetAttr (M:\src\dom\base\Element.cpp:2961)
#03: nsScrollbarFrame::UpdateChildrenAttributeValue (M:\src\layout\xul\nsScrollbarFrame.cpp:519)
#04: nsScrollbarFrame::AttributeChanged (M:\src\layout\xul\nsScrollbarFrame.cpp:165)
#05: mozilla::RestyleManager::AttributeChanged (M:\src\layout\base\RestyleManager.cpp:3663)
#06: mozilla::PresShell::AttributeChanged (M:\src\layout\base\PresShell.cpp:4596)
#07: mozilla::dom::MutationObservers::NotifyAttributeChanged (M:\src\dom\base\MutationObservers.cpp:152)
#08: mozilla::dom::Element::UnsetAttr (M:\src\dom\base\Element.cpp:2997)
#09: nsHTMLScrollFrame::SetScrollbarEnabled (M:\src\layout\generic\nsGfxScrollFrame.cpp:6745)
#10: nsHTMLScrollFrame::ReflowFinished (M:\src\layout\generic\nsGfxScrollFrame.cpp:6366)
#11: mozilla::PresShell::DidDoReflow (M:\src\layout\base\PresShell.cpp:9755)
#12: mozilla::PresShell::ProcessReflowCommands (M:\src\layout\base\PresShell.cpp:10162)
#13: mozilla::PresShell::DoFlushPendingNotifications (M:\src\layout\base\PresShell.cpp:4436)
#14: mozilla::dom::Document::FlushPendingNotifications (M:\src\dom\base\Document.cpp:10946)
#15: nsGenericHTMLElement::GetInnerText (M:\src\dom\html\nsGenericHTMLElement.cpp:3163)
#16: mozilla::dom::HTMLElement_Binding::get_innerText (M:\fx64-dbg\dom\bindings\HTMLElementBinding.cpp:378)

#01: mozilla::dom::Document::BeginUpdate (M:\src\dom\base\Document.cpp:7977)
#02: mozilla::dom::Element::SetAttr (M:\src\dom\base\Element.cpp:2569)
#03: nsHTMLScrollFrame::SetCoordAttribute (M:\src\layout\generic\nsGfxScrollFrame.cpp:6770)
#04: nsHTMLScrollFrame::UpdateScrollbarPosition (M:\src\layout\generic\nsGfxScrollFrame.cpp:5786)
#05: mozilla::PresShell::SetVisualViewportOffset (M:\src\layout\base\PresShell.cpp:11588)
#06: nsHTMLScrollFrame::ScrollToImpl (M:\src\layout\generic\nsGfxScrollFrame.cpp:3279)
#07: nsHTMLScrollFrame::CompleteAsyncScroll (M:\src\layout\generic\nsGfxScrollFrame.cpp:2361)
#08: nsHTMLScrollFrame::ScrollToWithOrigin (M:\src\layout\generic\nsGfxScrollFrame.cpp:2523)
#09: nsHTMLScrollFrame::ScrollToInternal (M:\src\layout\generic\nsGfxScrollFrame.cpp:2394)
#10: nsHTMLScrollFrame::ScrollTo (M:\src\layout\generic\nsGfxScrollFrame.h:257)
#11: mozilla::PresShell::ScrollFrameIntoView (M:\src\layout\base\PresShell.cpp:3863)
#12: mozilla::dom::Selection::ScrollIntoView (M:\src\dom\base\Selection.cpp:3618)
#13: mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run (M:\src\dom\base\Selection.cpp:3520)
#14: nsRefreshDriver::Tick (M:\src\layout\base\nsRefreshDriver.cpp:2695)

#01: mozilla::dom::Document::BeginUpdate (M:\src\dom\base\Document.cpp:7977)
#02: mozilla::dom::Element::SetAttr (M:\src\dom\base\Element.cpp:2569)
#03: nsScrollbarFrame::UpdateChildrenAttributeValue (M:\src\layout\xul\nsScrollbarFrame.cpp:538)
#04: nsScrollbarFrame::AttributeChanged (M:\src\layout\xul\nsScrollbarFrame.cpp:165)
#05: mozilla::RestyleManager::AttributeChanged (M:\src\layout\base\RestyleManager.cpp:3663)
#06: mozilla::PresShell::AttributeChanged (M:\src\layout\base\PresShell.cpp:4596)
#07: mozilla::dom::MutationObservers::NotifyAttributeChanged (M:\src\dom\base\MutationObservers.cpp:152)
#08: mozilla::dom::Element::SetAttrAndNotify (M:\src\dom\base\Element.cpp:2748)
#09: mozilla::dom::Element::SetAttr (M:\src\dom\base\Element.cpp:2586)
#10: nsHTMLScrollFrame::SetCoordAttribute (M:\src\layout\generic\nsGfxScrollFrame.cpp:6770)
#11: nsHTMLScrollFrame::UpdateScrollbarPosition (M:\src\layout\generic\nsGfxScrollFrame.cpp:5786)
#12: mozilla::PresShell::SetVisualViewportOffset (M:\src\layout\base\PresShell.cpp:11588)
#13: nsHTMLScrollFrame::ScrollToImpl (M:\src\layout\generic\nsGfxScrollFrame.cpp:3279)
#14: nsHTMLScrollFrame::CompleteAsyncScroll (M:\src\layout\generic\nsGfxScrollFrame.cpp:2361)
#15: nsHTMLScrollFrame::ScrollToWithOrigin (M:\src\layout\generic\nsGfxScrollFrame.cpp:2523)
#16: nsHTMLScrollFrame::ScrollToInternal (M:\src\layout\generic\nsGfxScrollFrame.cpp:2394)
#17: nsHTMLScrollFrame::ScrollTo (M:\src\layout\generic\nsGfxScrollFrame.h:257)
#18: mozilla::PresShell::ScrollFrameIntoView (M:\src\layout\base\PresShell.cpp:3863)
#19: mozilla::dom::Selection::ScrollIntoView (M:\src\dom\base\Selection.cpp:3618)
#20: mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run (M:\src\dom\base\Selection.cpp:3520)
#21: nsRefreshDriver::Tick (M:\src\layout\base\nsRefreshDriver.cpp:2695)

The demo causes a lot of scroll position changes at appending pasted content.

Okay, all things occur during the editor inserts content. Therefore, we can make IMEContentObserver stop notifying IME during that.

Assignee: nobody → masayuki
Severity: -- → S3
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All

For some reason, the text copied from the local text-editor triggers some DOMException on the testcase, and hence the text does not get syntax highlighted.

Uncaught DOMException: Index or size is negative or greater than the allowed amount
paintTokenHighlights https://bug1892514.bmoattachments.org/attachment.cgi?id=9398293:272
highlight https://bug1892514.bmoattachments.org/attachment.cgi?id=9398293:300
<anonymous> https://bug1892514.bmoattachments.org/attachment.cgi?id=9398293:317
2 attachment.cgi:272
paintTokenHighlights https://bug1892514.bmoattachments.org/attachment.cgi?id=9398293:272
highlight https://bug1892514.bmoattachments.org/attachment.cgi?id=9398293:300
<anonymous> https://bug1892514.bmoattachments.org/attachment.cgi?id=9398293:317

However, the text copied from Browser does get syntax highlighted, and scrolling through that causes checkerboarding.
https://share.firefox.dev/3We1iLM

I've encountered the highlights not appearing sometimes (mostly on linux fwiw), which seemed to be because the first pasted line is a comment. When I removed that line, highlighting appeared. Maybe you have the same issue?

Re. checkerboarding: The profile indicates that nsINode::IsSelected() is slow. I have some WIP patches that might help with that, see Bug 1867249.

Summary: Copy-paste of text from local text editor to Codepen demo is slow, but deleting that text is fast. Copy-paste of text from browser to Codepen demo is fast, but deelting that text is slow (STR in comment 8) → Copy-paste of text from local text editor to Codepen demo is slow, but deleting that text is fast. Copy-paste of text from browser to Codepen demo is fast, but deleting that text is slow (STR in comment 8)

RangeBoundaryBase which is the base class of RawNodePosition stores
previous sibling of a child node which the user wants to point. Therefore,
the user code is error prone and it's hard to understand for developers.

This patch creates some factory methods and they make the code clearer in
IMEContentObserver.

Depends on D211699

For centralizing the caching code this moves some calls of ContentEventHandler
methods into FlatTextCache. Remaining calls in IMEContentObserver will be
moved by the following patches.

Depends on D211701

The other, computing text length in a text node, is not required and enough
simple. Therefore, the following patches do not touch it.

Depends on D211702

Currently, there are no tests which check the text change notifications at
"undo". "undo" of our editor inserts nodes and removes nodes in the opposite
order from the order at "do" transactions. Therefore, we need to optimize more
for "undo" in the following patches.

Additionally, the following patches makes IMEContentObserver cache the text
length longer time and use its cache in more cases. Therefore, the assertions
will help to debug the code and enabling them only when running automated tests,
we can check regressions.

Finally, we should have logging code of the cache to investigate if one of the
assertions fail.

Depends on D211703

Currently, the caches are used only when they exactly match. However, the
direction of adding/removing siblings may be opposite from what current code
expects because they are designed for usual cases, but the undo feature of
our editor undoes things from last one to first one. Therefore, if text length
of adjacent sibling, we should compute the length from the data and avoid
computing it from start of the editing host since the range may be too long.

Depends on D211704

In the following patch, we need to keep caching newly added nodes longer.
Therefore, we'll need to manage the cache with more complicated checks.
So, making the complicated things encapsulated outside IMEContentObserver
keeps IMEContentObserver itself simpler.

Depends on D211707

Each DOM API call like nsINode::InsertNodeBefore creates a document change
transaction. Currently, IMEContentObserver flushes to pending text change
notification caused by new content nodes added in a document change when it
ends or begins another one. So, we need to make IMEContentObserver cache new
content nodes across document changes to save the number of flattened text
offset/length computation.

Currently, mAddedContentCache can store nodes in different containers to
improve the performance of setting innerHTML (bug 1250823). However, if we
keep the cache across multiple document changes, the nodes may be inserted
before the cached first content, and also the nodes may be inserted in the
range, additionally, the new ranges may be removed from the DOM again before
computing a text change notification. So, for supporting to cache new content
nodes across multiple document changes, we need to compute where new content
nodes and removed nodes are placed. Unfortunately, comparing 2 DOM node
positions is too expensive to run for each new content nodes and that makes
the fix of bug 1250823 slower. Therefore, this patch keeps the fix of
bug 1250823 as "consecutive nodes" mode of the cache, and adds new mode which
is called as "siblings" mode. If we cache only siblings in the same container,
the comparison cost is cheaper than nsContentUtils::ComparePoints. Therefore,
this patch uses the new mode when it caches new nodes across multiple document
changes.

When a document change ends or nested one starts, IMEContentObserver tries to
change the mode to "siblings" mode. If it's impossible (i.e., it stores nodes
in multiple containers), it computes a text change notification and post it.
Otherwise, it keeps using the cache with the "siblings" mode. Then, when new
content nodes are added before the editor ends handling current action,
IMEContentObserver checks whether the new nodes can be merged with the cached
range strictly and computes a text change notification if new nodes cannot be
stored with the cached range. In this case, the latest new content nodes are
ignored at computing the offset of the start of the cached range. Additionally,
when a content node is removed and the node is before the cached range, computes
a text change notification with adjusting offset of the start of the cached
range (adding the length of the removed content to the offset), or when a
content node is removed and the node is in the cached range, computes a text
change notification with adjusting length of the cached range (adding the length
of remove node).

Therefore, after applying this patch, IMEContentObserver starts to compute
offset and text length only a couple of times (at worst) for any number of new
text lines inserted by the editor because our editor inserts nodes as siblings
when inserting multiple lines. In other words, this patch does not optimize
the case when JS inserts a lot of nodes with calling DOM API with a loop. I
have no idea how to optimize this case, but currently there is no performance
bug for this. Therefore, I don't fix this now.

Depends on D211708

The preceding patches (D211697 and D211698) optimize
HTMLEditor::HandleInsertText to make the DOM mutations simpler for making
IMEContentObserver use cache to avoid to compute the similar flattened text
length a lot. However, there are not enough tests of text change notifications
in the cases. Therefore, before optimizing IMEContentObserver, we should have
more tests about that.

Depends on D211703

It's hard to write text change notification tests to verify the cache in
IMEContentObserver. Therefore, assertions helps to detect the bug of the
cache with existing automated tests. On the other hand, the verification
requires a lot of CPU time if editor inserts a lot of text, etc. Therefore,
I'd like to add assertions behind a pref and enable it in some specific tests
and entire the tests of editor/editing API.

Depends on D211833

This patch enables the assertions when running:

  • The nsITextInputProcessor tests
  • The IME composition tests under widget/
  • Under editor/libeditor/tests
  • Under editor/libeditor/crashtests
  • Under testing/web-platform/tests/editing`
  • Under testing/web-platform/tests/input-events`

Depends on D211834

Attachment #9404068 - Attachment description: Bug 1892514 - part 5: Add new automated test, add assertions to verify cache values and add log to investigate the behavior r=smaug! → Bug 1892514 - part 5-4: Add logging code to check the cache behavior of `IMEContentObserver` r=smaug!

I find an intermittent crash of delete-content-in-no-data-object.html due to an assertion failure of the new validation method. I can reproduce it with patched build of part 5-4 (i.e., without logic changes). So, it should be an unknown bug which exists in current build. The test sets <img>.innerHTML and I'm not sure whether it works as expected with ContentEventHandler. Therefore, I'll add it as knonw-failure if I don't find the cause quickly or the cause requires non-trivial changes.

Stack:

[6527] Assertion failure: mFlatTextLength == offset.inspect(), at /builds/worker/checkouts/gecko/dom/events/IMEContentObserver.cpp:2492
#01: mozilla::IMEContentObserver::FlatTextCache::AssertIfInvalidCache(mozilla::dom::Element const*) const [dom/events/IMEContentObserver.cpp:2492]
#02: mozilla::IMEContentObserver::FlatTextCache::ContentAdded(char const*, nsIContent const&, nsIContent const&, mozilla::Maybe<unsigned int> const&, mozilla::dom::Element const*) [dom/events/IMEContentObserver.cpp:2547]
#03: mozilla::IMEContentObserver::ContentAdded(nsINode*, nsIContent*, nsIContent*) [dom/events/IMEContentObserver.cpp:961]
#04: mozilla::dom::MutationObservers::NotifyContentAppended(nsIContent*, nsIContent*) [dom/base/MutationObservers.cpp:166]
#05: nsINode::InsertChildBefore(nsIContent*, nsIContent*, bool, mozilla::ErrorResult&) [dom/base/nsINode.cpp:0]
#06: nsContentUtils::SetNodeTextContent(nsIContent*, nsTSubstring<char16_t> const&, bool) [dom/base/nsContentUtils.cpp:5970]
#07: mozilla::dom::FragmentOrElement::SetInnerHTMLInternal(nsTSubstring<char16_t> const&, mozilla::ErrorResult&) [dom/base/FragmentOrElement.cpp:1970]

Waning:

WARNING: mFlatTextLength=18, offset: 17, mContainerNode:img.s.object.div.body.html.#document[designMode="on"] @ 0x7f0171b11080 (<object onerror="onError()">
BEFOREBEFORE<s>
</s></object><div align="center"><object onerror="onError()"><s><img>
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS
<kbd>
</kbd></s></object></div>
): file /builds/worker/checkouts/gecko/dom/events/IMEContentObserver.cpp:2490
Attachment #9404071 - Attachment description: Bug 1892514 - part 7: Make `IMEContentObserver::ContentRemoved` keeps `mEndOfAddedTextCache` if it's obviously valid r=smaug! → Bug 1892514 - part 7: Make `IMEContentObserver::ContentRemoved` keep `mEndOfAddedTextCache` if it's obviously valid r=smaug!
Attachment #9404074 - Attachment description: Bug 1892514 - part 10: Make `IMEContentObserver` use `mAddedContentCache` while editor handles an editing command r=smaug! → Bug 1892514 - part 10: Make `IMEContentObserver` keep using `mAddedContentCache` while editor handles an editing command r=smaug!

Starting from D211834, IMEContentObserver starts to validate its cache with
the test pref only in the debug build. Then,
delete-content-in-no-data-object.html starts crashing with new assertion
failure even without the patches after D211705 (i.e., without logic changes).
Therefore, this crash is an existing bug and out of scope of this bug.

Depends on D211709

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/71837d871833 part 1: Make `RawNodePosition` and `RawNodePositionBefore` have factory methods to explain where the creators point r=smaug https://hg.mozilla.org/integration/autoland/rev/dd42ed4c05f9 part 2: Clean up `IMEContentObserver::FlatTextCache` r=smaug https://hg.mozilla.org/integration/autoland/rev/beebf7370041 part 3: Move `ContentEventHandler` calls from `IMEContentObserver` to its `FlatTextCache` r=smaug https://hg.mozilla.org/integration/autoland/rev/c514cf8a7e6d part 4: Move the some remaining `ContentEventHandler` callers of `IMEContentObserver` to `FlatTextCache` r=smaug https://hg.mozilla.org/integration/autoland/rev/ae6dd25f6e60 part 5-1: Add new automated tests to test the text change notification when inserting multiple line text via `HTMLEditor` r=smaug https://hg.mozilla.org/integration/autoland/rev/6ed15cfce6df part 5-2: Add optional assertions into `IMEContentObserver::FlatTextCache` to detect creating invalid cache r=smaug https://hg.mozilla.org/integration/autoland/rev/1686b6177ab0 part 5-3: Enable the assertions in `IMEContentObserver::FlatTextCache` in some automated tests r=smaug https://hg.mozilla.org/integration/autoland/rev/e65d0b826f1d part 5-4: Add logging code to check the cache behavior of `IMEContentObserver` r=smaug https://hg.mozilla.org/integration/autoland/rev/d9cf5bfd4c34 part 6: Add accessors of `FlatTextCache::mFlatTextLength` r=smaug https://hg.mozilla.org/integration/autoland/rev/eb87fcf58d1a part 7: Make `IMEContentObserver::ContentRemoved` keep `mEndOfAddedTextCache` if it's obviously valid r=smaug https://hg.mozilla.org/integration/autoland/rev/6042902c7e2f part 8: Make `IMEContentObserver` keep `mEndOfAddedTextCache` and `mStartOfRemovingTextRangeCache` as far as possible when new nodes are inserted/appended r=smaug https://hg.mozilla.org/integration/autoland/rev/153feaf168c8 part 9: Make `IMEContentObserver` manage added content nodes with new struct r=smaug https://hg.mozilla.org/integration/autoland/rev/2b9fecca5d45 part 10: Make `IMEContentObserver` keep using `mAddedContentCache` while editor handles an editing command r=smaug https://hg.mozilla.org/integration/autoland/rev/56d8807b6c36 part 11: Mark `delete-content-in-no-data-object.html` as an intermittent crash test r=smaug

Backed out for causing bustage on IMEContentObserver.cpp

Backout link

Push with failures

Failure log

Flags: needinfo?(masayuki)

Hmm, why is it warned only in the specific build...?

Flags: needinfo?(masayuki)
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/2c9ecc9076f6 part 1: Make `RawNodePosition` and `RawNodePositionBefore` have factory methods to explain where the creators point r=smaug https://hg.mozilla.org/integration/autoland/rev/40df049d6562 part 2: Clean up `IMEContentObserver::FlatTextCache` r=smaug https://hg.mozilla.org/integration/autoland/rev/5c955c99b56c part 3: Move `ContentEventHandler` calls from `IMEContentObserver` to its `FlatTextCache` r=smaug https://hg.mozilla.org/integration/autoland/rev/1d34492b28f7 part 4: Move the some remaining `ContentEventHandler` callers of `IMEContentObserver` to `FlatTextCache` r=smaug https://hg.mozilla.org/integration/autoland/rev/833c65d741bb part 5-1: Add new automated tests to test the text change notification when inserting multiple line text via `HTMLEditor` r=smaug https://hg.mozilla.org/integration/autoland/rev/c3fc9232286c part 5-2: Add optional assertions into `IMEContentObserver::FlatTextCache` to detect creating invalid cache r=smaug https://hg.mozilla.org/integration/autoland/rev/a56158a3b83c part 5-3: Enable the assertions in `IMEContentObserver::FlatTextCache` in some automated tests r=smaug https://hg.mozilla.org/integration/autoland/rev/fcbd141bc1a1 part 5-4: Add logging code to check the cache behavior of `IMEContentObserver` r=smaug https://hg.mozilla.org/integration/autoland/rev/f29747daf8de part 6: Add accessors of `FlatTextCache::mFlatTextLength` r=smaug https://hg.mozilla.org/integration/autoland/rev/27dd1891fa92 part 7: Make `IMEContentObserver::ContentRemoved` keep `mEndOfAddedTextCache` if it's obviously valid r=smaug https://hg.mozilla.org/integration/autoland/rev/d9f3819f281b part 8: Make `IMEContentObserver` keep `mEndOfAddedTextCache` and `mStartOfRemovingTextRangeCache` as far as possible when new nodes are inserted/appended r=smaug https://hg.mozilla.org/integration/autoland/rev/bd2b11bf4de4 part 9: Make `IMEContentObserver` manage added content nodes with new struct r=smaug https://hg.mozilla.org/integration/autoland/rev/559c8515de2c part 10: Make `IMEContentObserver` keep using `mAddedContentCache` while editor handles an editing command r=smaug https://hg.mozilla.org/integration/autoland/rev/8e39756b37d7 part 11: Mark `delete-content-in-no-data-object.html` as an intermittent crash test r=smaug

Profile with Nightly that contains this fix : https://share.firefox.dev/3zuZg0p
(Time has reduced from 6s->1.6s)

Well, nsINode::ComputeIndexOf is still taking 0.5 sec in the profile, but it's called by EditorBase::TopLevelEditSubActionData::AddNodeToChangedRange, so, it's a performance bug of the editor module. In HandleInsertText, this is temporarily disabled. However, InsertNodeWithTransaction and CreateAndInsertElement start their own nested edit subaction. Therefore, the disablement does not work as expected by HandleInsertText. Anyway, this is a different bug.

QA Whiteboard: [qa-129b-p2]
See Also: → 1909171
Regressions: 1911010
See Also: → 1913253
Regressions: 1924232
See Also: → 2011011
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: