1375131 - Crash [@nsAutoOwningThread::AssertCurrentThreadOwnsMe]

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found while fuzzing mozilla-central rev 20170621-2b07ef4f3381.

ASAN:DEADLYSIGNAL
=================================================================
==10490==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000030 (pc 0x7ff77ee0645a bp 0x7ffe4e05b2c0 sp 0x7ffe4e05b2b0 T0)
==10490==The signal is caused by a READ memory access.
==10490==Hint: address points to the zero page.
    #0 0x7ff77ee06459 in nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const /home/worker/workspace/build/src/xpcom/base/nsISupportsImpl.cpp:41:7
    #1 0x7ff781b5f85c in AssertOwnership<24> /home/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:69:5
    #2 0x7ff781b5f85c in nsRange::AddRef() /home/worker/workspace/build/src/dom/base/nsRange.cpp:315
    #3 0x7ff7853c96fd in AddRef /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:37:11
    #4 0x7ff7853c96fd in AddRef /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:392
    #5 0x7ff7853c96fd in assign_with_AddRef /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:54
    #6 0x7ff7853c96fd in operator= /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:191
    #7 0x7ff7853c96fd in init<nsRange *> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/OwningNonNull.h:147
    #8 0x7ff7853c96fd in OwningNonNull /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/OwningNonNull.h:25
    #9 0x7ff7853c96fd in mozilla::HTMLEditRules::WillDeleteSelection(mozilla::dom::Selection*, short, short, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:2457
    #10 0x7ff7853c10d9 in mozilla::HTMLEditRules::WillDoAction(mozilla::dom::Selection*, mozilla::RulesInfo*, bool*, bool*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:650:14
    #11 0x7ff7854d8598 in mozilla::TextEditor::DeleteSelection(short, short) /home/worker/workspace/build/src/editor/libeditor/TextEditor.cpp:638:24
    #12 0x7ff785385b81 in mozilla::DeleteCommand::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/editor/libeditor/EditorCommands.cpp:697:18
    #13 0x7ff7836f4f85 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/dom/commandhandler/nsControllerCommandTable.cpp:147:26
    #14 0x7ff7836ebdcd in nsBaseCommandController::DoCommand(char const*) /home/worker/workspace/build/src/dom/commandhandler/nsBaseCommandController.cpp:136:25
    #15 0x7ff7836f2464 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/dom/commandhandler/nsCommandManager.cpp:212:22
    #16 0x7ff783c1218b in nsHTMLDocument::ExecCommand(nsAString const&, bool, nsAString const&, nsIPrincipal&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3345:18
    #17 0x7ff7831305dc in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:835:21
    #18 0x7ff783443dbe in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2960:13
    #19 0x7ff7899174a3 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #20 0x7ff7899174a3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1375131 - Part 1. Store ranges before using on loop.

This crash occurs that range is nullptr.

DeleteNonTableElements will update selection and range, so we shouldn't use range count before modifying selection.  So we should save range data for loop.

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

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1375131 - Part 2. Add crash test.

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

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1375131 - Part 3. Use stack class to save ranges before using on loop.

We have a possible loop like this bug.  So we should use stack class for loop.

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8880727 [details]
Bug 1375131 - Part 1. Store ranges before using on loop.

https://reviewboard.mozilla.org/r/152094/#review157082

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8880728 [details]
Bug 1375131 - Part 2. Add crash test.

https://reviewboard.mozilla.org/r/152096/#review157088

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Comment on attachment 8880729 [details]
Bug 1375131 - Part 3. Use stack class to save ranges before using on loop.

https://reviewboard.mozilla.org/r/152098/#review157090

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/094e8f3937e8
Part 1. Store ranges before using on loop. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/0c0db23bd493
Part 2. Add crash test. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/52f06c62dfe4
Part 3. Use stack class to save ranges before using on loop. r=masayuki

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back out for bustage in https://treeherder.mozilla.org/logviewer.html#?job_id=109514919&repo=autoland

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8adde8e79c55
Backed out changeset 52f06c62dfe4 
https://hg.mozilla.org/integration/autoland/rev/6adb1c9cd7ee
Backed out changeset 0c0db23bd493 
https://hg.mozilla.org/integration/autoland/rev/a966daa11b06
Backed out changeset 094e8f3937e8 for bustage

Comment 10

[Makoto Kato [:m_kato]]

(In reply to Carsten Book [:Tomcat] from comment #8)
> sorry had to back out for bustage in
> https://treeherder.mozilla.org/logviewer.html#?job_id=109514919&repo=autoland

Ah, I fotget explict keyword.  I will reland this tomorrow.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a46cfc0a8203
Part 1. Store ranges before using on loop. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/69bd67418866
Part 2. Add crash test. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/5baa008248ff
Part 3. Use stack class to save ranges before using on loop. r=masayuki

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a46cfc0a8203
https://hg.mozilla.org/mozilla-central/rev/69bd67418866
https://hg.mozilla.org/mozilla-central/rev/5baa008248ff

Comment 13

[Ryan VanderMeulen [:RyanVM]]

How far back does this issue go? Should we consider it for backport or is it good riding the 56 train?

Comment 14

[Makoto Kato [:m_kato]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> How far back does this issue go? Should we consider it for backport or is it
> good riding the 56 train?

Since crash rate is too low, I don't think that this should fix on 56.