Crash on null ptr [@ nsCOMPtr | ChangeAttributeTransaction]

RESOLVED FIXED in Firefox 53

Status

()

Core
Editor
P2
critical
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: truber, Assigned: m_kato)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla55
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

7 months ago
Created attachment 8851438 [details]
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
Flags: in-testsuite?
(Reporter)

Updated

7 months ago
Blocks: 336383
No longer blocks: 306663
(Assignee)

Updated

7 months ago
Crash Signature: [@ nsAttrAndChildArray::GetAttr ]
Priority: -- → P2
(Assignee)

Updated

7 months ago
Assignee: nobody → m_kato
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8851474 - Flags: review?(masayuki) → review+
Comment on attachment 8851475 [details]
Bug 1350772 - Part 2. Add test.

https://reviewboard.mozilla.org/r/123778/#review126192
Attachment #8851475 - Flags: review?(masayuki) → review+

Comment 5

7 months ago
Pushed by m_kato@ga2.so-net.ne.jp:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c762c4039731
https://hg.mozilla.org/mozilla-central/rev/89680c837876
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr52: --- → unaffected
Flags: in-testsuite? → in-testsuite+
Flags: needinfo?(m_kato)
(Assignee)

Comment 8

7 months ago
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
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+

Comment 10

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a10927531c5
https://hg.mozilla.org/releases/mozilla-aurora/rev/35d42d438c86
status-firefox54: affected → fixed

Comment 11

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/aa5538c3aa52
https://hg.mozilla.org/releases/mozilla-beta/rev/79bb8c332fd5
status-firefox53: affected → fixed
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.