1370737 - Crash in [@ nsPlainTextSerializer::AppendElementStart]

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

==67738==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f88eba68afc bp 0x7ffdf45a1c10 sp 0x7ffdf45a1ba0 T0)
==67738==The signal is caused by a READ memory access.
==67738==Hint: address points to the zero page.
    #0 0x7f88eba68afb in construct<bool, bool> src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/ext/new_allocator.h:120:4
    #1 0x7f88eba68afb in _M_push_back_aux<bool> src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/deque.tcc:456
    #2 0x7f88eba68afb in emplace_back<bool> src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/deque.tcc:142
    #3 0x7f88eba68afb in push_back src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/stl_deque.h:1413
    #4 0x7f88eba68afb in push src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/stl_stack.h:195
    #5 0x7f88eba68afb in nsPlainTextSerializer::AppendElementStart(mozilla::dom::Element*, mozilla::dom::Element*, nsAString&) src/dom/base/nsPlainTextSerializer.cpp:391
    #6 0x7f88eb9794e9 in nsDocumentEncoder::SerializeNodeStart(nsINode*, int, int, nsAString&, nsINode*) src/dom/base/nsDocumentEncoder.cpp:387:18
    #7 0x7f88eb97c88c in nsDocumentEncoder::SerializeRangeContextStart(nsTArray<nsINode*> const&, nsAString&) src/dom/base/nsDocumentEncoder.cpp:909:12
    #8 0x7f88eb97d484 in nsDocumentEncoder::SerializeRangeToString(nsRange*, nsAString&) src/dom/base/nsDocumentEncoder.cpp:992:8
    #9 0x7f88eb97eeae in nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsAString&) src/dom/base/nsDocumentEncoder.cpp:1136:12
    #10 0x7f88eb89a605 in SelectionCopyHelper(nsISelection*, nsIDocument*, bool, short, unsigned int, nsITransferable**) src/dom/base/nsCopySupport.cpp:182:22
    #11 0x7f88efc29852 in nsAutoCopyListener::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) src/layout/generic/nsSelection.cpp:7035:10
    #12 0x7f88efc09504 in mozilla::dom::Selection::NotifySelectionListeners() src/layout/generic/nsSelection.cpp:6507:28
    #13 0x7f88efc06244 in nsFrameSelection::NotifySelectionListeners(mozilla::SelectionType) src/layout/generic/nsSelection.cpp:2440:23
    #14 0x7f88efc1cd5a in mozilla::dom::Selection::AddRangeInternal(nsRange&, nsIDocument*, mozilla::ErrorResult&) src/layout/generic/nsSelection.cpp:5085:28
    #15 0x7f88efc1c7e8 in AddRange src/layout/generic/nsSelection.cpp:5031:10
    #16 0x7f88efc1c7e8 in mozilla::dom::Selection::AddRangeJS(nsRange&, mozilla::ErrorResult&) src/layout/generic/nsSelection.cpp:5025
    #17 0x7f88ec5e4e36 in mozilla::dom::SelectionBinding::addRange(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Selection*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/SelectionBinding.cpp:264:9
    #18 0x7f88ed323f5e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:2954:13
    #19 0x7f88f2e378d3 in CallJSNative src/js/src/jscntxtinlines.h:293:15
    #20 0x7f88f2e378d3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:470
    #21 0x7f88f2e2041b in CallFromStack src/js/src/vm/Interpreter.cpp:521:12
    #22 0x7f88f2e2041b in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3028
    #23 0x7f88f2e06648 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:410:12
    #24 0x7f88f2e37a58 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:488:15
    #25 0x7f88f2e38282 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:534:10
    #26 0x7f88f37a005b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2891:12
    #27 0x7f88ecd87247 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventListenerBinding.cpp:47:8
    #28 0x7f88ed6dbcaf in HandleEvent<mozilla::dom::EventTarget *> src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:65:12
    #29 0x7f88ed6dbcaf in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1143
    #30 0x7f88ed6ddbf1 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1320:20
    #31 0x7f88ed6bdbc1 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:464:16
    #32 0x7f88ed6c1092 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:825:9
    #33 0x7f88ed69022a in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp:894:12
    #34 0x7f88eba13cc1 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) src/dom/base/nsINode.cpp:1337:5
    #35 0x7f88eb565a3a in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString const&, bool, bool, bool, bool*, bool) src/dom/base/nsContentUtils.cpp:4366:18
    #36 0x7f88eb5657fb in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString const&, bool, bool, bool*) src/dom/base/nsContentUtils.cpp:4334:10
    #37 0x7f88eb9285e0 in nsDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5259:3
    #38 0x7f88eb9f5ff2 in applyImpl<nsDocument, void (nsDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1084:12
    #39 0x7f88eb9f5ff2 in apply<nsDocument, void (nsDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1090
    #40 0x7f88eb9f5ff2 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1133
    #41 0x7f88e8e9441e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1321:14
    #42 0x7f88e8ea0858 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:472:10
    #43 0x7f88e9c6cc91 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:96:21
    #44 0x7f88e9bc9d90 in RunInternal src/ipc/chromium/src/base/message_loop.cc:238:10
    #45 0x7f88e9bc9d90 in RunHandler src/ipc/chromium/src/base/message_loop.cc:231
    #46 0x7f88e9bc9d90 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:211
    #47 0x7f88ef0e068f in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
    #48 0x7f88f27a0ce1 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30
    #49 0x7f88f2971334 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4569:22
    #50 0x7f88f2972ea0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4749:8
    #51 0x7f88f29741f1 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4844:21
    #52 0x4eb5a3 in do_main src/browser/app/nsBrowserApp.cpp:236:22
    #53 0x4eb5a3 in main src/browser/app/nsBrowserApp.cpp:309
    #54 0x7f890482882f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
    #55 0x41d0f8 in _start (m-c-1496772615-asan-opt/firefox+0x41d0f8)

Comment 1

[Andrew Overholt [:overholt]]

Henri, can you please take a look here? It looks like Ehsan wrote this code in bug 1113238 so if you don't immediately see the problem we can ask him. Thank you!

Comment 2

[Andrew Overholt [:overholt]]

FWIW I don't get a crash when I click on the test case here on Windows 10 with the latest nightly. Should I, Tyson?

Comment 3

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Yes it should likely crash. Maybe there is a timing component? I've updated the test.

I tested on Ubuntu 16.04 with m-c 20170606181015 6cd0639e02ded.

Comment 4

[Andrew Overholt [:overholt]]

Hmm, still no crash for me on Windows. I can make it happen in Nightly on Fedora, too, though.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Like overholt, I can reproduce on Linux but not on Windows.

INFO: Got as far as we can go bisecting nightlies...
INFO: Last good revision: 9a8f2342fb3116d23989087e026448d38a3768c5 (2015-10-27)
INFO: First bad revision: fc706d376f0658e560a59c3dd520437b18e8c4a4 (2015-10-28)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9a8f2342fb3116d23989087e026448d38a3768c5&tochange=fc706d376f0658e560a59c3dd520437b18e8c4a4

Maybe bug 120684?

Comment 6

[Henri Sivonen (:hsivonen)]

I don't see anything obviously wrong in the Gecko stack frame. I see a bogus pointer (the contents of the pointer seem to be uninitialized memory) that, AFAICT, points to where the GNU C++ standard library tries to place stuff at. The bogus pointer seems to emerge from the implementation internals of the GNU C++ standard library instead of emerging from Gecko code in an obvious way.

The Linux vs. Windows difference is probably about GNU vs. Microsoft implementation of the C++ standard library.

Ehsan, do you have a reason to believe that this code needs the C++ standard library's space optimizations for bool? Or can we just replace this with nsAutoTArray and not debug further?

Comment 7

[(no longer active)]

Hmm, std::stack uses a deque by default and not a vector, and to the best of my knowledge std::deque<bool> should in no way be special...  Are you sure you're not thinking of vector<bool> (which this code isn't using)?

But at any rate, switching this to an AutoTArray is certainly fine, I can't remember why I used a stack<bool> here, probably because that was the easiest thing to type.  :/

Comment 8

[(no longer active)]

I debugged this.  The stack<bool> part is a distraction.  What's happening here is that there is an imbalance in the number of pushes and pops to the stack, and we pop once more than we push.

We end up coming from the script's call to Selection.addRange() to the selection copy helper code into the serializer helper machinery:

#1  0x00007f7bb7481b52 in nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsAString&) (this=0x7f7b9b0b8000, aMaxLength=0, aOutputString=<gNullChar> u"")
    at /home/ehsan/moz/src.1347035/dom/base/nsDocumentEncoder.cpp:1129
#2  0x00007f7bb7480f93 in nsDocumentEncoder::EncodeToString(nsAString&) (this=0x7f7b9b0b8000, aOutputString=<gNullChar> u"")
    at /home/ehsan/moz/src.1347035/dom/base/nsDocumentEncoder.cpp:1037
#3  0x00007f7bb7483d6c in nsHTMLCopyEncoder::EncodeToString(nsAString&) (this=0x7f7b9b0b8000, aOutputString=<gNullChar> u"")
    at /home/ehsan/moz/src.1347035/dom/base/nsDocumentEncoder.cpp:1489
#4  0x00007f7bb740bae9 in SelectionCopyHelper(nsISelection*, nsIDocument*, bool, short, unsigned int, nsITransferable**) (aSel=0x7f7b9b202ce0, aDoc=0x7f7b9b2fc000, doPutOnClipboard=true, aClipboardID=0, aFlags=524288, aTransferable=0x0) at /home/ehsan/moz/src.1347035/dom/base/nsCopySupport.cpp:182
#5  0x00007f7bb740b1de in nsCopySupport::HTMLCopy(nsISelection*, nsIDocument*, short, bool) (aSel=0x7f7b9b202ce0, aDoc=0x7f7b9b2fc000, aClipboardID=0, aWithRubyAnnotation=false)
    at /home/ehsan/moz/src.1347035/dom/base/nsCopySupport.cpp:308
#6  0x00007f7bb9ecdb4c in nsAutoCopyListener::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) (this=0x7f7b9c28ec00, aDoc=0x7f7b9b2fc370, aSel=0x7f7b9b202ce0, aReason=8) at /home/ehsan/moz/src.1347035/layout/generic/nsSelection.cpp:7035
#7  0x00007f7bb9ec0573 in mozilla::dom::Selection::NotifySelectionListeners() (this=0x7f7b9b202ce0) at /home/ehsan/moz/src.1347035/layout/generic/nsSelection.cpp:6507
#8  0x00007f7bb9ebe7e0 in nsFrameSelection::NotifySelectionListeners(mozilla::SelectionType) (this=0x7f7b9c468c60, aSelectionType=mozilla::SelectionType::eNormal)
    at /home/ehsan/moz/src.1347035/layout/generic/nsSelection.cpp:2440
#9  0x00007f7bb9ec802d in mozilla::dom::Selection::AddRangeInternal(nsRange&, nsIDocument*, mozilla::ErrorResult&) (this=0x7f7b9b202ce0, aRange=..., aDocument=0x7f7b9b2fc000, aRv=...) at /home/ehsan/moz/src.1347035/layout/generic/nsSelection.cpp:5085
#10 0x00007f7bb9ec7db0 in mozilla::dom::Selection::AddRange(nsRange&, mozilla::ErrorResult&) (this=0x7f7b9b202ce0, aRange=..., aRv=...)
    at /home/ehsan/moz/src.1347035/layout/generic/nsSelection.cpp:5031

The imbalance in caused by a <tbody> element for which only get a call to AppendElementEnd(), not AppendElementStart().  The reason why this happens is that when we get to here for the tbody element <https://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/dom/base/nsDocumentEncoder.cpp#354> IsVisibleNode() returns false because we don't have a frame <https://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/dom/base/nsDocumentEncoder.cpp#133> but when we get to <https://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/dom/base/nsDocumentEncoder.cpp#431> IsVisibleNode() now gets a frame and ends up returning true.

This screams like a missing flush, but the obvious fix of adding a flush before grabbing the frame didn't work.  I need to see why that is...

Comment 9

[(no longer active)]

One more interesting bit of finding is that the reason this bug is Linux only is that this is triggered by nsAutoCopyListener which is controlled by a Linux only pref to support copying the current selection to the selection clipboard:

<https://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/layout/generic/nsSelection.cpp#568>

Comment 10

[(no longer active)]

I think the proper fix is to notify the content serializer about all of the nodes we're entering and exiting even if they turn out to be invisible, since in the general case there isn't anything to guarantee that IsVisibleNode() will return the same value when we're about to call AppendElementStart() and AppendElementEnd()...

Comment 11

[(no longer active)]

Attachment: Track seen preformatted elements in the document encoder to maintain stack balance correctly irrespective of element visibility

Comment 12

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad4c197db0369caf19ba0679ebd780fdd23bb4f&selectedJob=106445207

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8876841 [details] [diff] [review]
Track seen preformatted elements in the document encoder to maintain stack balance correctly irrespective of element visibility

>+  bool              mNeedsPreformatScanning;

Please document this.

>+  NS_IMETHOD ScanElementForPreformat(mozilla::dom::Element* aElement) = 0;
>+  NS_IMETHOD ForgetElementForPreformat(mozilla::dom::Element* aElement) = 0;

And these.

r=me with that

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbb76f06ce1
Track seen preformatted elements in the document encoder to maintain stack balance correctly irrespective of element visibility; r=bzbarsky

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0fbb76f06ce1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Only a handful of crashes with this signature. Do you think it's worth requesting backport or should we just let it ride the trains?

Comment 17

[(no longer active)]

No, this isn't a safe patch to backport for beta, I'd rather wait.