Closed Bug 1370737 Opened 2 years ago Closed 2 years ago

Crash in [@ nsPlainTextSerializer::AppendElementStart]

Categories

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

44 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: tsmith, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-nullptr, testcase)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file test_case.html (obsolete) —
==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)
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!
Component: DOM → Serializers
Flags: needinfo?(hsivonen)
Priority: -- → P3
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?
Flags: needinfo?(twsmith)
Attached file 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.
Attachment #8875065 - Attachment is obsolete: true
Flags: needinfo?(twsmith)
Hmm, still no crash for me on Windows. I can make it happen in Nightly on Fedora, too, though.
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
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?
Crash Signature: [@ nsPlainTextSerializer::AppendElementStart]
Has Regression Range: --- → yes
Has STR: --- → irrelevant
Version: Trunk → 44 Branch
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?
Flags: needinfo?(hsivonen) → needinfo?(ehsan)
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.  :/
Flags: needinfo?(ehsan)
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...
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>
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()...
Blocks: 1113238
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
Attachment #8876841 - Flags: review?(bzbarsky) → review+
Pushed by eakhgari@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/0fbb76f06ce1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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?
Flags: needinfo?(ehsan)
Flags: in-testsuite+
No, this isn't a safe patch to backport for beta, I'd rather wait.
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.