Testcase found while fuzzing mozilla-central rev 20170302-d29f84406483. ASAN:DEADLYSIGNAL ================================================================= ==18961==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f0639e42d2d bp 0x7fff4e196440 sp 0x7fff4e196100 T0) #0 0x7f0639e42d2c in mozilla::HTMLEditRules::GetPromotedPoint(mozilla::HTMLEditRules::RulesEndpoint, nsIDOMNode*, int, EditAction, nsCOMPtr<nsIDOMNode>*, int*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:5422:16 #1 0x7f0639dd6875 in mozilla::HTMLEditRules::PromoteRange(nsRange&, EditAction) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:5657:3 #2 0x7f0639dd566a in mozilla::HTMLEditRules::AfterEditInner(EditAction, short) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:457:5 #3 0x7f0639dd4e56 in mozilla::HTMLEditRules::AfterEdit(EditAction, short) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:400:10 #4 0x7f0639e7e6bb in mozilla::HTMLEditor::EndOperation() /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:3515:25 #5 0x7f0639f06b02 in ~AutoRules /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EditorUtils.h:251:7 #6 0x7f0639f06b02 in mozilla::TextEditor::InsertLineBreak() /home/worker/workspace/build/src/editor/libeditor/TextEditor.cpp:761 #7 0x7f0639f03ccb in mozilla::TextEditor::TypedText(nsAString_internal const&, mozilla::TextEditor::ETypingAction) /home/worker/workspace/build/src/editor/libeditor/TextEditor.cpp:415:14 #8 0x7f0639e5d2a1 in mozilla::HTMLEditor::TypedText(nsAString_internal const&, mozilla::TextEditor::ETypingAction) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:1013:10 #9 0x7f0639da5820 in mozilla::InsertParagraphCommand::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/editor/libeditor/EditorCommands.cpp:1025:10 #10 0x7f06380ec3c6 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/dom/commandhandler/nsControllerCommandTable.cpp:147:10 #11 0x7f06380e2d8a in nsBaseCommandController::DoCommand(char const*) /home/worker/workspace/build/src/dom/commandhandler/nsBaseCommandController.cpp:136:10 #12 0x7f06380e96d6 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/dom/commandhandler/nsCommandManager.cpp:214:10 #13 0x7f06386a1bb8 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, nsIPrincipal&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3239:10 #14 0x7f0637b4fb6d in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:835:15 #15 0x7f0637dfc387 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2953:13

Comment on attachment 8844358 [details] Bug 1345015 - Part 1. Clean up GetPromotedPoint. https://reviewboard.mozilla.org/r/117848/#review119598 ::: editor/libeditor/HTMLEditRules.cpp:5422 (Diff revision 1) > + if (content) { > - *outNode = content->AsDOMNode(); > + *outNode = content->AsDOMNode(); > + } Although, I think that this must be necessary change, I don't think that this is enough to fix this bug. Currently, Selection.addRange() ignores selection limiter. Therefore, the testcase might be able to select all nodes in the document with: > var range = document.createRange(); > range.selectNode(documetn.documentElement); > document.getSelection().addRange(range); However, this selected range should not be replaced with anything. So, before handling input, we should shrink the selected range into root element (typically, document.body <https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/editor/libeditor/HTMLEditor.cpp#362,365-366,381>). So, I think that |HTMLEditRules::PromoteRange()| should check the selected range is in proper node. I think that you should shrink the range if start and/or end node is out of the root element. If you want to uplift this as far as safely, I'll give r+ for this patch with some fix of the testcase. Otherwise, could you try to fix it? I think that even with your patch, document.execCommand() in the testcause does nothing (or replacing the <body> with <p> or something). ::: editor/libeditor/crashtests/1345015.html:2 (Diff revision 1) > +<html> > + <head> Please use 2 spaces for indent. ::: editor/libeditor/crashtests/1345015.html:5 (Diff revision 1) > + o1 = window.getSelection(); > + o2 = document.createRange(); > + o3 = document.createRange(); > + o4 = document.createElement('div'); > + o5 = document.createElement('div'); > + document.documentElement.appendChild(o4); > + document.documentElement.appendChild(o5); > + o4.outerHTML = '<foo>'; > + o3.selectNode(o5); > + o1.addRange(o3); > + o3.setStart(o4, 0); > + o2.selectNode(document.documentElement); > + o1.addRange(o2); Hmm, I'd like you to give them better name with |var|. So, I think: o1: selection o2: range1 o3: range3 o4: div1 o5: div2 ::: editor/libeditor/crashtests/1345015.html:12 (Diff revision 1) > + o3 = document.createRange(); > + o4 = document.createElement('div'); > + o5 = document.createElement('div'); > + document.documentElement.appendChild(o4); > + document.documentElement.appendChild(o5); > + o4.outerHTML = '<foo>'; o4 is really necessary to reproduce this crash? ::: editor/libeditor/crashtests/1345015.html:13 (Diff revision 1) > + o3.selectNode(o5); > + o1.addRange(o3); > + o3.setStart(o4, 0); o3 (range selecting the second <div>) is really necessary for reproducing this crash? ::: editor/libeditor/crashtests/1345015.html:16 (Diff revision 1) > + o2.selectNode(document.documentElement); > + o1.addRange(o2); So, selected range is now, the entire of the document tree.

Comment on attachment 8844358 [details] Bug 1345015 - Part 1. Clean up GetPromotedPoint. https://reviewboard.mozilla.org/r/117848/#review119598 > Although, I think that this must be necessary change, I don't think that this is enough to fix this bug. > > Currently, Selection.addRange() ignores selection limiter. Therefore, the testcase might be able to select all nodes in the document with: > > > var range = document.createRange(); > > range.selectNode(documetn.documentElement); > > document.getSelection().addRange(range); > > However, this selected range should not be replaced with anything. So, before handling input, we should shrink the selected range into root element (typically, document.body <https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/editor/libeditor/HTMLEditor.cpp#362,365-366,381>). > > So, I think that |HTMLEditRules::PromoteRange()| should check the selected range is in proper node. I think that you should shrink the range if start and/or end node is out of the root element. > > If you want to uplift this as far as safely, I'll give r+ for this patch with some fix of the testcase. Otherwise, could you try to fix it? I think that even with your patch, document.execCommand() in the testcause does nothing (or replacing the <body> with <p> or something). OK, crash data isn't large volumes, so I change fix.

Comment on attachment 8844358 [details] Bug 1345015 - Part 1. Clean up GetPromotedPoint. https://reviewboard.mozilla.org/r/117848/#review134740 r- until coming the new patch.

dupe of bug 1267757 ?

Comment on attachment 8879052 [details] Bug 1345015 - Part 3. Add crash test. https://reviewboard.mozilla.org/r/150348/#review155086 ::: editor/libeditor/crashtests/1345015.html:4 (Diff revision 1) > + <script type="application/javascript"> > + document.designMode = "on"; > + let selection = window.getSelection(); > + > + let foo = document.createElement('foo'); > + let div = document.createElement('div'); > + document.documentElement.appendChild(foo); > + document.documentElement.appendChild(div); > + foo.outerHTML = '<foo>'; > + > + let range = document.createRange(); > + range.selectNode(div); > + selection.addRange(range); > + range.setStart(foo, 0); > + > + range = document.createRange(); > + range.selectNode(document.documentElement); > + selection.addRange(range); > + > + document.execCommand('insertparagraph', false, null); > + </script> nit: you use *4* spaces for indent only in <script>

Comment on attachment 8844358 [details] Bug 1345015 - Part 1. Clean up GetPromotedPoint. https://reviewboard.mozilla.org/r/117848/#review155096 ::: editor/libeditor/HTMLEditRules.cpp:5457 (Diff revision 2) > if (actionID == EditAction::insertText || > actionID == EditAction::insertIMEText || > actionID == EditAction::insertBreak || > actionID == EditAction::deleteText) { > bool isSpace, isNBSP; > - nsCOMPtr<nsIContent> content = do_QueryInterface(node), temp; > + nsCOMPtr<nsIContent> content = aNode.AsContent(); nsCOMPtr<nsIContent> content = aNode.IsContent() ? aNode.AsContent() : nullptr; # AsContent() assumes that the caller guarantees that the node always has nsIContent. ::: editor/libeditor/HTMLEditRules.cpp:5496 (Diff revision 2) > if (aWhere == kStart) { > // some special casing for text nodes > if (node->IsNodeOfType(nsINode::eTEXT)) { > if (!node->GetParentNode()) { > // Okay, can't promote any further > - return; > + return EditorDOMPoint(&aNode, aOffset); Please use node and offset here because this code is really explicit what this return now. However, node and offset may be modified before here in the future. Then, this code could cause unexpected bug (i.e., the developer may not realize this "solid" return statement). ::: editor/libeditor/HTMLEditRules.cpp:5560 (Diff revision 2) > // aWhere == kEnd > // some special casing for text nodes > if (node->IsNodeOfType(nsINode::eTEXT)) { > if (!node->GetParentNode()) { > // Okay, can't promote any further > - return; > + return EditorDOMPoint(&aNode, aOffset); Same. Please use node and offset here. ::: editor/libeditor/HTMLEditRules.cpp:5696 (Diff revision 2) > - nsCOMPtr<nsIDOMNode> opDOMStartNode; > - nsCOMPtr<nsIDOMNode> opDOMEndNode; > - int32_t opStartOffset, opEndOffset; > - > + EditorDOMPoint opStart = > + GetPromotedPoint(kStart, *startNode, startOffset, aOperationType); > + EditorDOMPoint opEnd = > + GetPromotedPoint(kEnd, *endNode, endOffset, aOperationType); Hmm, startNode and endNode are not checked if they are nullptr. In here, both of them can be nullptr. So, could you check aRange.IsPositioned() above? If it's not positioned, this should (can) do nothing.

Comment on attachment 8879051 [details] Bug 1345015 - Part 2. Don't promote range when selection node isn't content. https://reviewboard.mozilla.org/r/150346/#review155104 ::: editor/libeditor/HTMLEditRules.cpp:5692 (Diff revision 1) > + if (aOperationType == EditAction::insertText || > + aOperationType == EditAction::insertIMEText || > + aOperationType == EditAction::insertBreak || > + aOperationType == EditAction::deleteText) { > + if (NS_WARN_IF(!startNode->IsContent()) || > + NS_WARN_IF(!endNode->IsContent())) { > + return; > + } > + } Sorry, I don't understand this change. Could you explain why do we need to check the edit action or remove it if it does not make sense. (It's okay if you are not sure whether they are the all what should be handled specially. Even if so, I'd like you to add explanation here because others (including me) will be confused whether they can add other edit action into this condition. So, "not sure" comment is helpful in such case.) ::: editor/libeditor/HTMLEditRules.cpp:5696 (Diff revision 1) > + if (NS_WARN_IF(!startNode->IsContent()) || > + NS_WARN_IF(!endNode->IsContent())) { I don't think that we should output warnings here because I assume that this case occurs with your automated test. So, you should add comment here like that startNode and/or endNode can be a document node.

Comment on attachment 8844358 [details] Bug 1345015 - Part 1. Clean up GetPromotedPoint. https://reviewboard.mozilla.org/r/117848/#review155096 > nsCOMPtr<nsIContent> content = aNode.IsContent() ? aNode.AsContent() : nullptr; > > # AsContent() assumes that the caller guarantees that the node always has nsIContent. This root cause of this issue is that HTMLEditRules::GetPromotedPoint doesn't support non-content on text transaction such as insertText etc. If non-content node, it returns null. So I would like to use AsContent for assertion check. If callee passes non-content node for text transaction, we should detect it. HTMLEditRules::GetPromotedPoint shouldn't return null node. > Hmm, startNode and endNode are not checked if they are nullptr. In here, both of them can be nullptr. So, could you check aRange.IsPositioned() above? If it's not positioned, this should (can) do nothing. OK.

s/If callee passes non-content node/If caller passes non-content node/ Also, so, I added simple null check by previous fix, but you suggest that we should check caller by previous review

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/autoland/rev/7a792bc76e13 Part 1. Clean up GetPromotedPoint. r=masayuki https://hg.mozilla.org/integration/autoland/rev/99b10a5f5a3c Part 2. Don't promote range when selection node isn't content. r=masayuki https://hg.mozilla.org/integration/autoland/rev/8012d7f5aede Part 3. Add crash test. r=masayuki

https://hg.mozilla.org/mozilla-central/rev/7a792bc76e13 https://hg.mozilla.org/mozilla-central/rev/99b10a5f5a3c https://hg.mozilla.org/mozilla-central/rev/8012d7f5aede

Is this something we should consider for backport or can it ride the 56 train?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20) > Is this something we should consider for backport or can it ride the 56 > train? There is a few crash data (It may be that all crashes are my test only) by this. So I don't think that this should be uplifted.