Closed Bug 1402904 Opened 2 years ago Closed 2 years ago

crash near null [@ IsList]

Categories

(Core :: DOM: Editor, defect, P1, critical)

34 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: tsmith, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(4 keywords)

Crash Data

Attachments

(3 files)

Attached file test_case.html
==19448==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001c (pc 0x7f52078a07ca bp 0x7ffda11b19b0 sp 0x7ffda11b1680 T0)
==19448==The signal is caused by a READ memory access.
==19448==Hint: address points to the zero page.
    #0 0x7f52078a07c9 in GetBoolFlag /src/dom/base/nsINode.h:1615:12
    #1 0x7f52078a07c9 in IsElement /src/dom/base/nsINode.h:457
    #2 0x7f52078a07c9 in IsHTMLElement /src/dom/base/nsINode.h:634
    #3 0x7f52078a07c9 in IsAnyOfHTMLElements<nsIAtom *, nsIAtom *, nsIAtom *> /src/dom/base/nsINode.h:645
    #4 0x7f52078a07c9 in IsList /src/editor/libeditor/HTMLEditUtils.cpp:270
    #5 0x7f52078a07c9 in mozilla::HTMLEditRules::WillHTMLIndent(mozilla::dom::Selection*, bool*, bool*) /src/editor/libeditor/HTMLEditRules.cpp:4003
    #6 0x7f520784dc3d in WillIndent /src/editor/libeditor/HTMLEditRules.cpp:3701:19
    #7 0x7f520784dc3d in mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) /src/editor/libeditor/HTMLEditRules.cpp:658
    #8 0x7f52078c5f64 in mozilla::HTMLEditor::Indent(nsTSubstring<char16_t> const&) /src/editor/libeditor/HTMLEditor.cpp:2145:24
    #9 0x7f52079ab7aa in nsIndentCommand::DoCommand(char const*, nsISupports*) /src/editor/composer/nsComposerCommands.cpp:503:22
    #10 0x7f5205b39345 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /src/dom/commandhandler/nsControllerCommandTable.cpp:147:26
    #11 0x7f5205b2ff7e in nsBaseCommandController::DoCommand(char const*) /src/dom/commandhandler/nsBaseCommandController.cpp:136:25
    #12 0x7f5205b36634 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /src/dom/commandhandler/nsCommandManager.cpp:212:22
    #13 0x7f5206060e30 in nsHTMLDocument::ExecCommand(nsTSubstring<char16_t> const&, bool, nsTSubstring<char16_t> const&, nsIPrincipal&, mozilla::ErrorResult&) /src/dom/html/nsHTMLDocument.cpp:3334:18
    #14 0x7f5205592429 in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:891:21
    #15 0x7f5205872550 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
    #16 0x7f520bee6ff4 in CallJSNative /src/js/src/jscntxtinlines.h:293:15
    #17 0x7f520bee6ff4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495
...
see log.txt
Flags: in-testsuite?
On debug builds, this also hits the assert below:
Assertion failure: aNode, at z:/build/build/src/editor/libeditor/HTMLEditUtils.cpp:269

Regression range:
INFO: Last good revision: ffdd1a398105 (2014-08-20)
INFO: First bad revision: dac8b4a0bd7c (2014-08-21)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ffdd1a398105&tochange=dac8b4a0bd7c

Probably one of bug 1054735 or bug 1055032.
Crash Signature: [@ IsList]
Component: DOM → Editor
Keywords: assertion
Version: Trunk → 34 Branch
bughunter reproduced similar crashes with nsINode::IsHTMLElement nsINode::IsAnyOfHTMLElements<nsIAtom*, nsIAtom*, nsIAtom*> mozilla::HTMLEditUtils::IsList mozilla::HTMLEditRules::WillHTMLIndent mozilla::HTMLEditRules::WillIndent on the stack.

An official Nightly build on fedora crashed with:

bp-f4a42425-ca63-4660-88f8-5e2ec0170927
[@ nsINode::IsAnyOfHTMLElements<T> ]
Crash Signature: [@ IsList] → [@ IsList] [@ nsINode::IsHTMLElement() ] [@ nsINode::IsAnyOfHTMLElements<T> ]
Priority: -- → P1
Assignee: nobody → m_kato
Comment on attachment 8915484 [details]
Bug 1402904 - Part 1. Some operations should check whether parent node of each selected node is null.

https://reviewboard.mozilla.org/r/186694/#review191856

::: commit-message-dedd9:3
(Diff revision 1)
> +Bug 1402904 - Part 1. Some operations should check whether parent node of each selected node is null. r?masayuki
> +
> +GetNodesForOperation returns node array from current selection.  So a node into this array might have no parent node.  So if no parent node, we should ignore it.

s/a node into this array/some nodes in this array

/So if no parent node, we should ignore it/We should ignore such orphans

::: commit-message-dedd9:5
(Diff revision 1)
> +Bug 1402904 - Part 1. Some operations should check whether parent node of each selected node is null. r?masayuki
> +
> +GetNodesForOperation returns node array from current selection.  So a node into this array might have no parent node.  So if no parent node, we should ignore it.
> +
> +GetNodesForOperation might have to reject node that parent is nothing.  How do you think it?

I agree with that or, GetPromotedRanges() might have to ignore selection ranges which is not in the document.

Anyway, such change is risky. If you think we should take your patches in Beta, please keep current approach. This looks safer.

And also, please remove this line before landing.

::: editor/libeditor/HTMLEditRules.cpp:4013
(Diff revision 1)
>      if (!mHTMLEditor->IsEditable(curNode)) {
>        continue;
>      }
>  
> -    curParent = curNode->GetParentNode();
> -    int32_t offset = curParent ? curParent->IndexOf(curNode) : -1;
> +    int32_t offset;
> +    nsCOMPtr<nsINode> curParent =

nit: GetNodeLocation() returns raw pointer, not already_AddRefed. So,

if (!EditorBase::GetNodeLocation(curNode, &offset)) {
  continue;
}

is simpler and faster.

::: editor/libeditor/HTMLEditRules.cpp:4806
(Diff revision 1)
> -    nsCOMPtr<nsINode> curParent = curNode->GetParentNode();
> -    int32_t offset = curParent ? curParent->IndexOf(curNode) : -1;
> +    int32_t offset;
> +    nsCOMPtr<nsINode> curParent =
> +      EditorBase::GetNodeLocation(curNode, &offset);
> +    if (!curParent) {
> +      continue;
> +    }

Same.

::: editor/libeditor/HTMLEditRules.cpp:8795
(Diff revision 1)
> -    int32_t offset = curParent ? curParent->IndexOf(curNode) : -1;
> +    nsCOMPtr<nsINode> curParent =
> +      EditorBase::GetNodeLocation(curNode, &offset);
> +    if (!curParent) {
> +      continue;
> +    }

Same.
Attachment #8915484 - Flags: review?(masayuki) → review+
Comment on attachment 8915485 [details]
Bug 1402904 - Part 2. Add crash tests.

https://reviewboard.mozilla.org/r/186696/#review191858

::: editor/libeditor/crashtests/1405747.html:3
(Diff revision 1)
> +try { htmlvar00017.addEventListener("DOMSubtreeModified", eventhandler5); } catch(e) { }
> +try { htmlvar00017.align = ""; } catch(e) { }

Could you indent these lines?

::: editor/libeditor/crashtests/1405747.html:7
(Diff revision 1)
> +try { document.execCommand("selectAll", false); } catch(e) { }
> +try { document.execCommand("justifyCenter", false); } catch(e) { }
> +try { document.execCommand("forwardDelete", false); } catch(e) { }

ditto.
Attachment #8915485 - Flags: review?(masayuki) → review+
Comment on attachment 8915485 [details]
Bug 1402904 - Part 2. Add crash tests.

https://reviewboard.mozilla.org/r/186696/#review191858

> Could you indent these lines?

If I modify this sample even a little, this crash doesn't occur :-<.
Comment on attachment 8915485 [details]
Bug 1402904 - Part 2. Add crash tests.

https://reviewboard.mozilla.org/r/186696/#review191858

> If I modify this sample even a little, this crash doesn't occur :-<.

Wow, really?? That's really wired, but okay to land this as-is.
Has Regression Range: --- → no
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/0e684e74a413
Part 1. Some operations should check whether parent node of each selected node is null. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/daa2f3495cee
Part 2. Add crash tests. r=masayuki
Backed out for build bustage at editor/libeditor/HTMLEditRules.cpp:4068:

https://hg.mozilla.org/integration/autoland/rev/930c00802a0110d62743a17feda1f50c2abe5bc0
https://hg.mozilla.org/integration/autoland/rev/fc7159c2cd73feb90ccb224648882730748c379a

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=daa2f3495ceef50fca612d0da44a86aa8b754e1f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=135655574&repo=autoland

/builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:4068:78: error: invalid initialization of non-const reference of type 'nsCOMPtr<nsINode>&' from an rvalue of type 'nsCOMPtr<nsINode>'
/builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:4112:36: error: invalid initialization of non-const reference of type 'nsCOMPtr<nsINode>&' from an rvalue of type 'nsCOMPtr<nsINode>'
/builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:4142:71: error: invalid initialization of non-const reference of type 'nsCOMPtr<nsINode>&' from an rvalue of type 'nsCOMPtr<nsINode>'
/builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:4859:60: error: invalid initialization of non-const reference of type 'nsCOMPtr<nsINode>&' from an rvalue of type 'nsCOMPtr<nsINode>'
/builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:8818:78: error: invalid initialization of non-const reference of type 'nsCOMPtr<nsINode>&' from an rvalue of type 'nsCOMPtr<nsINode>'
/builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:8861:36: error: invalid initialization of non-const reference of type 'nsCOMPtr<nsINode>&' from an rvalue of type 'nsCOMPtr<nsINode>'
/builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:8890:64: error: invalid initialization of non-const reference of type 'nsCOMPtr<nsINode>&' from an rvalue of type 'nsCOMPtr<nsINode>'
Flags: needinfo?(m_kato)
Flags: needinfo?(m_kato)
> nit: GetNodeLocation() returns raw pointer, not already_AddRefed. So,

Cannot. SplitAsNeeded uses nsCOMPtr<nsINode> as parameter.
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4902df2916ee
Part 1. Some operations should check whether parent node of each selected node is null. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b0ccdd2fa5
Part 2. Add crash tests. r=masayuki
Duplicate of this bug: 1405747
Given that this crash signature doesn't seem to be hitting in the wild, I'm going to mark 57 as wontfix under the assumption that this can just ride the trains. Please set it back to affected and request Beta approval if I'm missing something, though!
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.