Closed Bug 1402469 Opened 7 years ago Closed 7 years ago

crash near null in [@ mozilla::HTMLEditor::GetFirstEditableChild]

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

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

People

(Reporter: tsmith, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-nullptr, testcase)

Crash Data

Attachments

(3 files)

test_case.html
261 bytes, text/html
Details
Bug 1402469 - Part 1. Return value of ConvertListType should use Element instead of nsresult.
59 bytes, text/x-review-board-request
masayuki
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1402469 - Part 2. Add crash test.
59 bytes, text/x-review-board-request
masayuki
: review+
Sylvestre
: approval-mozilla-beta+
Details
Attached file test_case.html — 
==13026==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000048 (pc 0x7f251ad26085 bp 0x7ffdb30fee90 sp 0x7ffdb30fede0 T0)
    #0 0x7f251ad26084 in nsCOMPtr_base /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:288:60
    #1 0x7f251ad26084 in nsCOMPtr /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:460
    #2 0x7f251ad26084 in mozilla::HTMLEditor::GetFirstEditableChild(nsINode&) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:4203
    #3 0x7f251ad0f94e in mozilla::HTMLEditor::RemoveBlockContainer(nsIContent&) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:3845:32
    #4 0x7f251acd2193 in mozilla::HTMLEditRules::WillMakeList(mozilla::dom::Selection*, nsAString_internal const*, bool, nsAString_internal const*, bool*, bool*, nsAString_internal const*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:3153:14
    #5 0x7f251acc518b in mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:621:14
    #6 0x7f251ad58cc5 in mozilla::HTMLEditor::MakeOrChangeList(nsAString_internal const&, bool, nsAString_internal const&) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:1988:17
    #7 0x7f251ae3092b in nsListCommand::ToggleState(nsIEditor*) /home/worker/workspace/build/src/editor/composer/nsComposerCommands.cpp:305:10
    #8 0x7f251ae2cb69 in nsBaseStateUpdatingCommand::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/editor/composer/nsComposerCommands.cpp:92:10
    #9 0x7f251c2ac146 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/embedding/components/commandhandler/nsControllerCommandTable.cpp:147:10
    #10 0x7f251c2a2b2a in nsBaseCommandController::DoCommand(char const*) /home/worker/workspace/build/src/embedding/components/commandhandler/nsBaseCommandController.cpp:136:10
    #11 0x7f251c2a9486 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/embedding/components/commandhandler/nsCommandManager.cpp:214:10
    #12 0x7f2519648fbe in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3234:10
    #13 0x7f2518ac9ae4 in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:829:15
    #14 0x7f2518e66399 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2904:13
    #15 0x7f251f1ec345 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #16 0x7f251f1ec345 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #17 0x7f251f1cc74f in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:510:12
    #18 0x7f251f1cc74f in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2922
    #19 0x7f251f1b190d in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:405:12
    #20 0x7f251f1ec9af in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:477:15
    #21 0x7f251f1ecff2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #22 0x7f251ecbde4d in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2828:12
    #23 0x7f251884f96f in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:259:37
    #24 0x7f251928008a in Call<nsISupports *> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:361:12
    #25 0x7f251928008a in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /home/worker/workspace/build/src/dom/events/JSEventHandler.cpp:214
    #26 0x7f251924a42d in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /home/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1134:16
    #27 0x7f251924be57 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /home/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1287:17
    #28 0x7f2519236bb6 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:380:5
    #29 0x7f251923a248 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:711:9
    #30 0x7f251b429bfc in nsDocumentViewer::LoadComplete(nsresult) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1047:7
    #31 0x7f251c1ce9ab in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7635:5
    #32 0x7f251c1ca7b4 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7439:7
    #33 0x7f251c1d1e1f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7336:13
    #34 0x7f2516364700 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:1255:3
    #35 0x7f2516363698 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:840:5
    #36 0x7f25163603f8 in nsDocLoader::DocLoaderIsEmpty(bool) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:730:9
    #37 0x7f25163624f4 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:612:5
    #38 0x7f25163630ac in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:468:14
    #39 0x7f25148b6eba in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:633:18
    #40 0x7f2517359db6 in nsDocument::DoUnblockOnload() /home/worker/workspace/build/src/dom/base/nsDocument.cpp:8647:7
    #41 0x7f251735968e in nsDocument::UnblockOnload(bool) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:8575:9
    #42 0x7f251732e5bb in nsDocument::DispatchContentLoadedEvents() /home/worker/workspace/build/src/dom/base/nsDocument.cpp:5061:3
    #43 0x7f25173f38a2 in applyImpl<nsDocument, void (nsDocument::*)()> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:775:12
    #44 0x7f25173f38a2 in apply<nsDocument, void (nsDocument::*)()> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:781
    #45 0x7f25173f38a2 in mozilla::detail::RunnableMethodImpl<void (nsDocument::*)(), true, false>::Run() /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:810
    #46 0x7f25146d749b in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1216:7
    #47 0x7f25147595dc in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:361:10
    #48 0x7f251551225f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #49 0x7f2515483db8 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #50 0x7f2515483db8 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:225
    #51 0x7f2515483db8 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:205
    #52 0x7f251ab19c5f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #53 0x7f251cb97221 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #54 0x7f251cd2e537 in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4488:10
    #55 0x7f251cd2fcad in XREMain::XRE_main(int, char**, nsXREAppData const*) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4621:8
    #56 0x7f251cd30b6c in XRE_main /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4712:16
    #57 0x4df91a in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:282:10
    #58 0x4df91a in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:415
    #59 0x7f253044b82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #60 0x41ba88 in _start (firefox+0x41ba88)
Flags: in-testsuite?
INFO: Last good revision: 1461a4071341c282afcf7b72e33036412d2251d4 (2016-05-01)
INFO: First bad revision: 77cead2cd20300623eea2416bc9bce4d5021df09 (2016-05-02)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1461a4071341c282afcf7b72e33036412d2251d4&tochange=77cead2cd20300623eea2416bc9bce4d5021df09

Tons of nsHTMLEditor patches in there...
Crash Signature: [@ mozilla::HTMLEditor::GetFirstEditableChild]
Has Regression Range: --- → yes
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
Component: DOM → Editor
Blocks: 1053779
Comment on attachment 8911660 [details]
Bug 1402469 - Part 1. Return value of ConvertListType should use Element instead of nsresult.

https://reviewboard.mozilla.org/r/183060/#review188236

::: editor/libeditor/HTMLEditRules.cpp:3285
(Diff revision 1)
>    nsCOMPtr<nsINode> curParent;
>    nsCOMPtr<Element> curList, prevListItem;
>  
>    for (uint32_t i = 0; i < listCount; i++) {
>      // here's where we actually figure out what to do
>      nsCOMPtr<Element> newBlock;

Looks like that newBlock is used only for temporarily use cases. So, perhaps, you can declare this in each block (3 blocks). But out of scope this bug. Up to you, if you do it here.

::: editor/libeditor/HTMLEditRules.cpp:4499
(Diff revision 1)
>  HTMLEditRules::ConvertListType(Element* aList,
> -                               Element** aOutList,
>                                 nsIAtom* aListType,
>                                 nsIAtom* aItemType)

Well, I'm afraid that are mutation observers locked during this call? mHTMLEditor or aList could be gone during this call...

(Out of scope of this bug, though.)
Attachment #8911660 - Flags: review?(masayuki) → review+
Comment on attachment 8911661 [details]
Bug 1402469 - Part 2. Add crash test.

https://reviewboard.mozilla.org/r/183062/#review188242

::: editor/libeditor/crashtests/1402469.html:3
(Diff revision 1)
> +<script type="application/javascript">
> +function do_test() {
> +  document.execCommand("insertUnorderedList", false);
> +}
> +</script>
> +</head>
> +<body onload="do_test()">
> +<table>
> +  <th contenteditable="true">
> +    <ol contenteditable="false">
> +  </th>

Hmm, when you add this kind of test, perhaps, you can write to check if the command works as expected. Then, we can check both the behavior and avoiding crash.  However, this is okay to use this.
Attachment #8911661 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/5efdecc08685
Part 1. Return value of ConvertListType should use Element instead of nsresult. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/5e311cd7074e
Part 2. Add crash test. r=masayuki
Safe to say this can just ride the 58 train and not worry about backports?
status-firefox-esr52: affected → wontfix
Flags: needinfo?(m_kato)
Flags: in-testsuite?
Flags: in-testsuite+
Comment on attachment 8911660 [details]
Bug 1402469 - Part 1. Return value of ConvertListType should use Element instead of nsresult.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1053779

[User impact if declined]:
Firefox crashes by specific script.

[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]:
Nothing

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Add null check only to detect error.

[String changes made/needed]:
No
Flags: needinfo?(m_kato)
Attachment #8911660 - Flags: approval-mozilla-beta?
Attachment #8911661 - Flags: approval-mozilla-beta?
Comment on attachment 8911660 [details]
Bug 1402469 - Part 1. Return value of ConvertListType should use Element instead of nsresult.

Fix a crash, taking it.
Should be in 57b5
Attachment #8911660 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8911661 [details]
Bug 1402469 - Part 2. Add crash test.

Fix a crash, taking it.
Should be in 57b5
Attachment #8911661 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Makoto Kato [:m_kato] from comment #10)
> [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

Setting qe-verify- based on Makoto Kato's assessment on manual testing needs 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.

Attachment

General

Creator:
Created:
Updated:
Size: