1350772 - Crash on null ptr [@ nsCOMPtr | ChangeAttributeTransaction]

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase crashes on mozilla-central revision f5e214144799.

==18346==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fa8fa1cb943 bp 0x7ffe42bf5890 sp 0x7ffe42bf5850 T0)
==18346==The signal is caused by a READ memory access.
==18346==Hint: address points to the zero page.
    #0 0x7fa8fa1cb942 in nsCOMPtr /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:464:7
    #1 0x7fa8fa1cb942 in ChangeAttributeTransaction /home/worker/workspace/build/src/editor/libeditor/ChangeAttributeTransaction.cpp:21
    #2 0x7fa8fa1cb942 in mozilla::EditorBase::CreateTxnForRemoveAttribute(mozilla::dom::Element&, nsIAtom&) /home/worker/workspace/build/src/editor/libeditor/EditorBase.cpp:4267
    #3 0x7fa8fa1cb7b0 in mozilla::EditorBase::RemoveAttribute(mozilla::dom::Element*, nsIAtom*) /home/worker/workspace/build/src/editor/libeditor/EditorBase.cpp:1265:5
    #4 0x7fa8fa2903c4 in mozilla::HTMLEditRules::SplitParagraph(nsIDOMNode*, nsIContent*, mozilla::dom::Selection*, nsCOMPtr<nsIDOMNode>*, int*) /home/worker/workspace/build/src/editor/libeditor/HTML
EditRules.cpp:6509:21
    #5 0x7fa8fa25fbe6 in mozilla::HTMLEditRules::ReturnInParagraph(mozilla::dom::Selection*, nsIDOMNode*, nsIDOMNode*, int, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRul
es.cpp:6461:10
    #6 0x7fa8fa231c0d in mozilla::HTMLEditRules::WillInsertBreak(mozilla::dom::Selection&, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:1562:7
    #7 0x7fa8fa22eff0 in mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:631:14
    #8 0x7fa8fa346ba5 in mozilla::TextEditor::InsertLineBreak() /home/worker/workspace/build/src/editor/libeditor/TextEditor.cpp:710:24
    #9 0x7fa8fa34453b in mozilla::TextEditor::TypedText(nsAString const&, mozilla::TextEditor::ETypingAction) /home/worker/workspace/build/src/editor/libeditor/TextEditor.cpp:419:14
    #10 0x7fa8fa2a45fe in mozilla::HTMLEditor::TypedText(nsAString const&, mozilla::TextEditor::ETypingAction) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:1012:22
    #11 0x7fa8fa1f6300 in mozilla::InsertParagraphCommand::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/editor/libeditor/EditorCommands.cpp:1025:22
    #12 0x7fa8f8618d25 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/dom/commandhandler/nsControllerCommandTable.cpp:147:26
    #13 0x7fa8f860f89d in nsBaseCommandController::DoCommand(char const*) /home/worker/workspace/build/src/dom/commandhandler/nsBaseCommandController.cpp:136:25
    #14 0x7fa8f8616104 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/dom/commandhandler/nsCommandManager.cpp:214:22
    #15 0x7fa8f8b5aee1 in nsHTMLDocument::ExecCommand(nsAString const&, bool, nsAString const&, nsIPrincipal&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3313
:18
    #16 0x7fa8f80b41ac in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/d
om/bindings/HTMLDocumentBinding.cpp:835:21

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1350772 - Part 1. Check whether node can be splited.

At first, HTMLEditor::GetActiveEditingHost might return null in this situation, we should check whether nullptr is returned.   At second, SplitNodeDeep returns error since curent is design mode and selection node has no parent.  So we should check error.

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

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1350772 - Part 2. Add test.

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

Comment 3

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

Comment on attachment 8851474 [details]
Bug 1350772 - Part 1. Check whether node can be splited.

https://reviewboard.mozilla.org/r/123776/#review126190

::: editor/libeditor/HTMLEditRules.cpp:1527
(Diff revision 1)
>    // If the active editing host is an inline element, or if the active editing
>    // host is the block parent itself, just append a br.
>    nsCOMPtr<Element> host = htmlEditor->GetActiveEditingHost();
> -  if (!EditorUtils::IsDescendantOf(blockParent, host)) {
> +  if (host && !EditorUtils::IsDescendantOf(blockParent, host)) {

Although, it's not your fault, could you update the comment? Because I was confused at reading the comment at checking if "else" case after you changed is correct.

Perhaps,

// When there is an active editing host (the <body> if it's in designMode)
// and a block which becomes the parent of line breaker is in it, do the
// standard thing.

::: editor/libeditor/HTMLEditRules.cpp:6494
(Diff revision 1)
>    *aSelNode = GetAsDOMNode(selNode);
>    NS_ENSURE_SUCCESS(rv, rv);
>    // split the paragraph
>    NS_ENSURE_STATE(mHTMLEditor);
>    NS_ENSURE_STATE(selNode->IsContent());
> -  mHTMLEditor->SplitNodeDeep(*para, *selNode->AsContent(), *aOffset,
> +  int32_t offset = mHTMLEditor->SplitNodeDeep(*para, *selNode->AsContent(), *aOffset,

Looks like this line becomes over 80 characters. Please break after "=" and start next line under first "t" of int32_t.

Comment 4

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

Comment on attachment 8851475 [details]
Bug 1350772 - Part 2. Add test.

https://reviewboard.mozilla.org/r/123778/#review126192

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c762c4039731
Part 1. Check whether node can be splited. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/89680c837876
Part 2. Add test. r=masayuki

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/c762c4039731
https://hg.mozilla.org/mozilla-central/rev/89680c837876

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Confirmed regression from bug 1324996 via mozregression. Please request Aurora/Beta approval on this when you get a chance. The patch grafts cleanly to both branches.

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8851474 [details]
Bug 1350772 - Part 1. Check whether node can be splited.

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1324996

[User impact if declined]:
When HTML page uses design mode, document.execCommand("insertParagraph") will crashes when selected element is out of document.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Low.

[Why is the change risky/not risky?]:
Add parameter check when returning error

[String changes made/needed]:
No

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8851474 [details]
Bug 1350772 - Part 1. Check whether node can be splited.

Fix a crash. Aurora54+ & Beta53+.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a10927531c5
https://hg.mozilla.org/releases/mozilla-aurora/rev/35d42d438c86

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa5538c3aa52
https://hg.mozilla.org/releases/mozilla-beta/rev/79bb8c332fd5

Comment 12

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Makoto's assessment on manual testing needs (see Comment 8) and the fact that this fix has automated coverage.