Closed
Bug 1370737
Opened 7 years ago
Closed 7 years ago
Crash in [@ nsPlainTextSerializer::AppendElementStart]
Categories
(Core :: DOM: Serializers, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: tsmith, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
(Keywords: crash, csectype-nullptr, testcase)
Crash Data
Attachments
(2 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
==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•7 years ago
|
||
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
Comment 2•7 years ago
|
||
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)
Reporter | ||
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
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
Comment 5•7 years ago
|
||
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
status-firefox53:
--- → wontfix
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox-esr45:
--- → wontfix
status-firefox-esr52:
--- → affected
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)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
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...
Assignee | ||
Comment 9•7 years ago
|
||
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>
Assignee | ||
Comment 10•7 years ago
|
||
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()...
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8876841 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•7 years ago
|
||
Assignee: nobody → ehsan
Comment 13•7 years ago
|
||
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+
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 16•7 years ago
|
||
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+
Assignee | ||
Comment 17•7 years ago
|
||
No, this isn't a safe patch to backport for beta, I'd rather wait.
Flags: needinfo?(ehsan)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•