Closed Bug 1350772 Opened 6 years ago Closed 6 years ago

Crash on null ptr [@ nsCOMPtr | ChangeAttributeTransaction]


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




Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed


(Reporter: truber, Assigned: m_kato)



(Keywords: crash, testcase)

Crash Data


(3 files)

Attached file 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
    #5 0x7fa8fa25fbe6 in mozilla::HTMLEditRules::ReturnInParagraph(mozilla::dom::Selection*, nsIDOMNode*, nsIDOMNode*, int, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRul
    #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
    #16 0x7fa8f80b41ac in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/d
Flags: in-testsuite?
Blocks: 336383
No longer blocks: stirdom
Crash Signature: [@ nsAttrAndChildArray::GetAttr ]
Priority: -- → P2
Assignee: nobody → m_kato
Comment on attachment 8851474 [details]
Bug 1350772 - Part 1. Check whether node can be splited.

::: 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.


// 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(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.
Attachment #8851474 - Flags: review?(masayuki) → review+
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.
Blocks: 1324996
Flags: in-testsuite? → in-testsuite+
Flags: needinfo?(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?]:

[Has the fix been verified in Nightly?]:

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

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

[Is the change risky?]:

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

[String changes made/needed]:
Flags: needinfo?(m_kato)
Attachment #8851474 - Flags: approval-mozilla-beta?
Attachment #8851474 - Flags: approval-mozilla-aurora?
Comment on attachment 8851474 [details]
Bug 1350772 - Part 1. Check whether node can be splited.

Fix a crash. Aurora54+ & Beta53+.
Attachment #8851474 - Flags: approval-mozilla-beta?
Attachment #8851474 - Flags: approval-mozilla-beta+
Attachment #8851474 - Flags: approval-mozilla-aurora?
Attachment #8851474 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on Makoto's assessment on manual testing needs (see Comment 8) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.