1425997 - Assertion failure: aPoint.IsSetAndValid(), at /src/editor/libeditor/WSRunObject.cpp:1401

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Assertion failure: aPoint.IsSetAndValid(), at /src/editor/libeditor/WSRunObject.cpp:1401

#0 mozilla::WSRunObject::GetNextCharPoint(mozilla::EditorDOMPointBase<nsINode*, nsIContent*> const&) /src/editor/libeditor/WSRunObject.cpp:1403:36
#1 mozilla::WSRunObject::GetAsciiWSBounds(short, nsINode*, int, mozilla::dom::Text**, int*, mozilla::dom::Text**, int*) /src/editor/libeditor/WSRunObject.cpp:1545:21
#2 mozilla::WSRunObject::ConvertToNBSP(mozilla::WSRunObject::WSPoint) /src/editor/libeditor/WSRunObject.cpp:1513:3
#3 mozilla::WSRunObject::PrepareToDeleteRangePriv(mozilla::WSRunObject*) /src/editor/libeditor/WSRunObject.cpp:1261:23
#4 mozilla::WSRunObject::PrepareToDeleteRange(mozilla::HTMLEditor*, nsCOMPtr<nsINode>*, int*, nsCOMPtr<nsINode>*, int*) /src/editor/libeditor/WSRunObject.cpp:135:20
#5 mozilla::HTMLEditRules::WillDeleteSelection(mozilla::dom::Selection*, short, short, bool*, bool*) /src/editor/libeditor/HTMLEditRules.cpp:2645:10
#6 mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) /src/editor/libeditor/HTMLEditRules.cpp:653:14
#7 mozilla::TextEditor::DeleteSelection(short, short) /src/editor/libeditor/TextEditor.cpp:671:24
#8 mozilla::DeleteCommand::DoCommand(char const*, nsISupports*) /src/editor/libeditor/EditorCommands.cpp:791:22
#9 nsControllerCommandTable::DoCommand(char const*, nsISupports*) /src/dom/commandhandler/nsControllerCommandTable.cpp:147:26
#10 nsBaseCommandController::DoCommand(char const*) /src/dom/commandhandler/nsBaseCommandController.cpp:136:25
#11 nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /src/dom/commandhandler/nsCommandManager.cpp:212:22
#12 nsHTMLDocument::ExecCommand(nsTSubstring<char16_t> const&, bool, nsTSubstring<char16_t> const&, nsIPrincipal&, mozilla::ErrorResult&) /src/dom/html/nsHTMLDocument.cpp:3282:18
#13 mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:849:21
#14 mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3042:13
#15 0x3b8cdf227f10  (<unknown module>)

Comment 1

[Makoto Kato [:m_kato]]

assertion only.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Hmm, this is one of existing bug of mutation observer hell.  When we try to replace ASCII whitespaces with an NBSP, the mutation observer removes the inserted NBSP.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90c4e86505815193f248d27dcd048e4860a27a11

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1425997 - Don't try to remove whitespaces in WSRunObject::ConvertToNBSP() when the text node is changed by mutation observer

WSRunObject::ConvertToNBSP() inserts an NBSP, then, removes following ASCII
whitespaces.  When inserting an NBSP, mutation observer may change the
text node.  In this case, it shouldn't keep working on removing ASCII
whitespaces because it may causes unexpected result.

This patch also renames ConvertToNBSP() and GetAsciiWSBounds() to
InsertNBSPAndRemoveFollowingASCIIWhitespaces() and
GetASCIIWhitespacesBounds() for making their jobs clearer.

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

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8938753 [details]
Bug 1425997 - Don't try to remove whitespaces in WSRunObject::ConvertToNBSP() when the text node is changed by mutation observer

https://reviewboard.mozilla.org/r/209300/#review215120

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/900d89625a37
Don't try to remove whitespaces in WSRunObject::ConvertToNBSP() when the text node is changed by mutation observer r=m_kato

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/900d89625a37

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

:masayuki, I was looking at the test test_bug1425997.html and noticed that it doesn't test anything, it just as todo_is() calls.  Is there a reason to run this test if we are not actually looking for anything?  Maybe it is looking for a crash or assertions?

https://searchfox.org/mozilla-central/source/editor/libeditor/tests/test_bug1425997.html

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #9)
> :masayuki, I was looking at the test test_bug1425997.html and noticed that
> it doesn't test anything, it just as todo_is() calls.  Is there a reason to
> run this test if we are not actually looking for anything?  Maybe it is
> looking for a crash or assertions?
> 
> https://searchfox.org/mozilla-central/source/editor/libeditor/tests/
> test_bug1425997.html

Yeah, it's testing assertions. If this bug is regressed, it detects with hitting MOZ_ASSERT on debug build.