1402904 - crash near null [@ IsList]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith] (PTO)]

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

Comment 1

[(PTO June 19-July 7) Ryan VanderMeulen]

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.

Comment 2

[Bob Clary [:bc] (inactive)]

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> ]

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1402904 - Part 1. Some operations should check whether parent node of each selected node is null.

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?

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

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 1402904 - Part 2. Add crash tests.

1405747.html is kind of same issue.  But when editing 1405747.html, the crash doesn't occur.  So I keep original HTML by jsfuzzy.

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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.

Comment 7

[Makoto Kato [:m_kato]]

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 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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.

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8915484 [details]
Bug 1402904 - Part 1. Some operations should check whether parent node of each selected node is null.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186694/diff/1-2/

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8915485 [details]
Bug 1402904 - Part 2. Add crash tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186696/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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>'

Comment 13

[Makoto Kato [:m_kato]]

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

Cannot. SplitAsNeeded uses nsCOMPtr<nsINode> as parameter.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4902df2916ee
https://hg.mozilla.org/mozilla-central/rev/f0b0ccdd2fa5

Comment 17

[(PTO June 19-July 7) Ryan VanderMeulen]

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!