1402196 - crash near null in [@ nsContentSubtreeIterator::Init]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

==22787==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000b0 (pc 0x7f2d18397c02 bp 0x7ffe74462e30 sp 0x7ffe74462e20 T0)
==22787==The signal is caused by a READ memory access.
==22787==Hint: address points to the zero page.
    #0 0x7f2d18397c01 in IsPositioned /src/dom/base/nsRange.h:117:12
    #1 0x7f2d18397c01 in nsContentSubtreeIterator::Init(nsIDOMRange*) /src/dom/base/nsContentIterator.cpp:1337
    #2 0x7f2d1bf30298 in mozilla::DOMSubtreeIterator::Init(nsRange&) /src/editor/libeditor/EditorUtils.cpp:126:17
    #3 0x7f2d1bf7c957 in mozilla::HTMLEditRules::GetNodesForOperation(nsTArray<RefPtr<nsRange> >&, nsTArray<mozilla::OwningNonNull<nsINode> >&, EditAction, mozilla::HTMLEditRules::TouchContent) /src/editor/libeditor/HTMLEditRules.cpp:5816:24
    #4 0x7f2d1bf78773 in GetNodesFromSelection /src/editor/libeditor/HTMLEditRules.cpp:6266:17
    #5 0x7f2d1bf78773 in mozilla::HTMLEditRules::GetListActionNodes(nsTArray<mozilla::OwningNonNull<nsINode> >&, mozilla::HTMLEditRules::EntireList, mozilla::HTMLEditRules::TouchContent) /src/editor/libeditor/HTMLEditRules.cpp:5956
    #6 0x7f2d1bf67c1c in mozilla::HTMLEditRules::WillMakeList(mozilla::dom::Selection*, nsTSubstring<char16_t> const*, bool, nsTSubstring<char16_t> const*, bool*, bool*, nsTSubstring<char16_t> const*) /src/editor/libeditor/HTMLEditRules.cpp:3210:8
    #7 0x7f2d1bf5acaa in mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) /src/editor/libeditor/HTMLEditRules.cpp:655:14
    #8 0x7f2d1bfe87b8 in mozilla::HTMLEditor::MakeOrChangeList(nsTSubstring<char16_t> const&, bool, nsTSubstring<char16_t> const&) /src/editor/libeditor/HTMLEditor.cpp:1937:24
    #9 0x7f2d1c0b6e98 in nsListCommand::ToggleState(mozilla::HTMLEditor*) /src/editor/composer/nsComposerCommands.cpp:332:23
    #10 0x7f2d1c0b4296 in nsBaseStateUpdatingCommand::DoCommand(char const*, nsISupports*) /src/editor/composer/nsComposerCommands.cpp:105:10
    #11 0x7f2d1a240a15 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /src/dom/commandhandler/nsControllerCommandTable.cpp:147:26
    #12 0x7f2d1a23764e in nsBaseCommandController::DoCommand(char const*) /src/dom/commandhandler/nsBaseCommandController.cpp:136:25
    #13 0x7f2d1a23dd04 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /src/dom/commandhandler/nsCommandManager.cpp:212:22
    #14 0x7f2d1a76a7a3 in nsHTMLDocument::ExecCommand(nsTSubstring<char16_t> const&, bool, nsTSubstring<char16_t> const&, nsIPrincipal&, mozilla::ErrorResult&) /src/dom/html/nsHTMLDocument.cpp:3334:18
    #15 0x7f2d19c97d79 in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:891:21
    #16 0x7f2d19f79ad0 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
    #17 0x7f2d205ebb54 in CallJSNative /src/js/src/jscntxtinlines.h:293:15
    #18 0x7f2d205ebb54 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495
    #19 0x7f2d205d59b6 in CallFromStack /src/js/src/vm/Interpreter.cpp:546:12
    #20 0x7f2d205d59b6 in Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3084
    #21 0x7f2d205bcd7b in js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:435:12
    #22 0x7f2d205ebcec in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:513:15
    #23 0x7f2d205ec642 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:559:10
    #24 0x7f2d2104272b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2965:12
    #25 0x7f2d199aeaa5 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:260:37
    #26 0x7f2d1a3854f5 in Call<nsISupports *> /src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12
    #27 0x7f2d1a3854f5 in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /src/dom/events/JSEventHandler.cpp:215
    #28 0x7f2d1a34eac9 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1112:51
    #29 0x7f2d1a350b90 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
    #30 0x7f2d1a330981 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
    #31 0x7f2d1a333e52 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
    #32 0x7f2d1a301f8a in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) /src/dom/events/EventDispatcher.cpp:891:12
    #33 0x7f2d1854b001 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) /src/dom/base/nsINode.cpp:1341:5
    #34 0x7f2d18057f7a in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, bool, bool, bool, bool*, bool) /src/dom/base/nsContentUtils.cpp:4514:18
    #35 0x7f2d18057d3b in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, bool, bool, bool*) /src/dom/base/nsContentUtils.cpp:4482:10
    #36 0x7f2d1c15184d in mozilla::css::SheetLoadData::FireLoadEvent(nsIThreadInternal*) /src/layout/style/Loader.cpp:309:3
    #37 0x7f2d1c151bcc in AfterProcessNextEvent /src/layout/style/Loader.cpp:292:3
    #38 0x7f2d1c151bcc in non-virtual thunk to mozilla::css::SheetLoadData::AfterProcessNextEvent(nsIThreadInternal*, bool) /src/layout/style/Loader.cpp:286
    #39 0x7f2d1590708c in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1047:3
    #40 0x7f2d1590ca8c in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
    #41 0x7f2d166b13d1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
    #42 0x7f2d1661329b in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10
    #43 0x7f2d1661329b in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319
    #44 0x7f2d1661329b in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299
    #45 0x7f2d1bdc2c7f in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
    #46 0x7f2d1ff20b61 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #47 0x7f2d201016ab in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
    #48 0x7f2d201032a8 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
    #49 0x7f2d201046db in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
    #50 0x4ebea3 in do_main /src/browser/app/nsBrowserApp.cpp:236:22
    #51 0x4ebea3 in main /src/browser/app/nsBrowserApp.cpp:309
    #52 0x7f2d332c582f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #53 0x41d9f8 in _start (firefox+0x41d9f8)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Masayuki touched relevant code in bug 1375502, perhaps related?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Perhaps, no.

Crashed at inline method and according to the address, accessing it with nullptr.
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/base/nsRange.h#117

However, there is a nullptr check before referring nsRange::IsPositioned():
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/base/nsContentIterator.cpp#313,317,321-322

I don't understand what occurs.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, I referred wrong method, it's nsContentSubtreeIterator::Init, not nsContentIterator::Init

https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/base/nsContentIterator.cpp#1330,1332,1336-1337

Yes, no nullptr check here!

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

RangeItem::GetRange() may return nullptr:
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/editor/libeditor/SelectionState.cpp#663,666,668

However, the RangeItems are initialized with nsRange:
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/editor/libeditor/HTMLEditRules.cpp#5788,5793-5794,5796,5807,5809

So, nsRange::SetStartAndEnd() shouldn't fail...


How do you reproduce this bug?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that this is regression of bug 1345015.

Each selection ranges may be adjusted in HTMLEditRules::GetPromotedRanges:
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/editor/libeditor/HTMLEditRules.cpp#5622,5629,5633,5638,5641

However, it may fail in opt build:
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/editor/libeditor/HTMLEditRules.cpp#5717-5720

Then, HTMLEditRules::GetPromotedRanges includes non-positioned ranges. That will cause RangeItem::GetRange() returning nullptr.

It's easy to avoid the crash, but this crash detects unexpected bug in HTMLEditRules::GetPromotedRanges, probably.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, bug 1149163 (part 7), not bug 1345015.

Comment 7

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Oops I guess I forgot to attach the test case, sorry.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Seems Editor is a better module fit for this issue.

Comment 9

[Makoto Kato [:m_kato]]

Attachment: Bug 1402196 - Part 1. Don't add null range to aArrayOfRanges.

RangeItem::GetRange can return null, so we shouldn't add null range to
aArrayOfRanges.

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

Comment 10

[Makoto Kato [:m_kato]]

Attachment: Bug 1402196 - Part 2. Add crashtest.

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

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8920930 [details]
Bug 1402196 - Part 1. Don't add null range to aArrayOfRanges.

https://reviewboard.mozilla.org/r/191866/#review197090

Oh, sorry, I completely forgot this bug. Thank you for your work!

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8920931 [details]
Bug 1402196 - Part 2. Add crashtest.

https://reviewboard.mozilla.org/r/191868/#review197092

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3aff5bcb5816
Part 1. Don't add null range to aArrayOfRanges. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/10e5f407149d
Part 2. Add crashtest. r=masayuki

Comment 14

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

Backed out for misconfig in editor/libeditor/crashtests/crashtests.list line 91: unknown test type 1402196.html:

https://hg.mozilla.org/integration/autoland/rev/51a6e6d8ce203daf6e1bea0fe0e2fc5e2640661a
https://hg.mozilla.org/integration/autoland/rev/2d9463ac3b6024350c5622b6dd1bbff8ac3d9b64

Push with failures (original push was landed on a busted tree): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9c968cad8e6ae47aec9ec71d5ace795c2468a82b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139114918&repo=autoland

> REFTEST ERROR | EXCEPTION: Error in manifest file file:///builds/worker/workspace/build/tests/reftest/tests/editor/libeditor/crashtests/crashtests.list line 91: unknown test type 1402196.html

Comment 15

[Makoto Kato [:m_kato]]

It has typo...

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9553b43be35
Part 1. Don't add null range to aArrayOfRanges. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/5920c5afdc82
Part 2. Add crashtest. r=masayuki

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/b9553b43be35
https://hg.mozilla.org/mozilla-central/rev/5920c5afdc82

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem like a must-fix for 57. Feel free to set the status back to affected and nominate for approval if you feel strongly otherwise.