InvalidArrayIndex_CRASH in [@ SVGTextFrame::GetRotationOfChar]

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tsmith, Assigned: jwatt)

Tracking

(Blocks 1 bug, {assertion, crash, testcase})

Trunk
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57- verified, firefox58+ fixed)

Details

Attachments

(2 attachments)

Posted file test_case.html
==109584==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000550926 bp 0x7ffe46033250 sp 0x7ffe460330e0 T0)
==109584==The signal is caused by a WRITE memory access.
==109584==Hint: address points to the zero page.
    #0 0x550925 in MOZ_CrashPrintf /builds/worker/workspace/build/src/mfbt/Assertions.cpp:63:3
    #1 0x7fda18230ebf in InvalidArrayIndex_CRASH(unsigned long, unsigned long) /builds/worker/workspace/build/src/xpcom/ds/nsTArray.cpp:26:3
    #2 0x7fda1f5189ed in ElementAt /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1048:7
    #3 0x7fda1f5189ed in operator[] /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1086
    #4 0x7fda1f5189ed in SVGTextFrame::GetStartPositionOfChar(nsIContent*, unsigned int, mozilla::nsISVGPoint**) /builds/worker/workspace/build/src/layout/svg/SVGTextFrame.cpp:4321
    #5 0x7fda1dc8fa9e in mozilla::dom::SVGTextContentElement::GetStartPositionOfChar(unsigned int, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/svg/SVGTextContentElement.cpp:167:19
    #6 0x7fda1bb5f185 in mozilla::dom::SVGTextContentElementBinding::getStartPositionOfChar(JSContext*, JS::Handle<JSObject*>, mozilla::dom::SVGTextContentElement*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/SVGTextContentElementBinding.cpp:194:58
    #7 0x7fda1c981a10 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3055:13
    #8 0x7fda22ff11d4 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #9 0x7fda22ff11d4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495
    #10 0x7fda22fdad6f in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12
    #11 0x7fda22fdad6f in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084
    #12 0x7fda22fc22fb in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12
    #13 0x7fda22ff136c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:513:15
    #14 0x7fda22ff1cc2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:559:10
    #15 0x7fda23a46fbb in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2965:12
    #16 0x7fda1c3b69e5 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:260:37
    #17 0x7fda1cd8d435 in Call<nsISupports *> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12
    #18 0x7fda1cd8d435 in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /builds/worker/workspace/build/src/dom/events/JSEventHandler.cpp:215
    #19 0x7fda1cd56a09 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1112:51
    #20 0x7fda1cd58ad0 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1283:20
    #21 0x7fda1cd388c1 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:462:16
    #22 0x7fda1cd3bd92 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:822:9
    #23 0x7fda1f01625e in nsDocumentViewer::LoadComplete(nsresult) /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1081:7
    #24 0x7fda21fbee21 in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7749:21
    #25 0x7fda21fbae44 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7547:7
    #26 0x7fda21fc27ef in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7444:13
    #27 0x7fda19cbce90 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:1320:3
    #28 0x7fda19cbbf4c in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:861:14
    #29 0x7fda19cb8f06 in nsDocLoader::DocLoaderIsEmpty(bool) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:750:9
    #30 0x7fda19cbad45 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:632:5
    #31 0x7fda19cbb9ac in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:488:14
    #32 0x7fda184bee5d in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:629:28
    #33 0x7fda1ae8abed in nsDocument::DoUnblockOnload() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:9173:18
    #34 0x7fda1ae8a7b1 in nsDocument::UnblockOnload(bool) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:9095:9
    #35 0x7fda1ae63bf9 in nsDocument::DispatchContentLoadedEvents() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:5599:3
    #36 0x7fda1af047e2 in applyImpl<nsDocument, void (nsDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1142:12
    #37 0x7fda1af047e2 in apply<nsDocument, void (nsDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1148
    #38 0x7fda1af047e2 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1192
    #39 0x7fda1831191d in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14
    #40 0x7fda18317658 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:521:10
    #41 0x7fda190bb9b1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #42 0x7fda1901d87b in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #43 0x7fda1901d87b in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #44 0x7fda1901d87b in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #45 0x7fda1e7c965f in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #46 0x7fda22923a21 in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #47 0x7fda22b0456b in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4701:22
    #48 0x7fda22b06168 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4865:8
    #49 0x7fda22b0759b in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4960:21
    #50 0x4ebea3 in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:236:22
    #51 0x4ebea3 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:309
    #52 0x7fda35d1b82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #53 0x41d9f8 in _start (firefox+0x41d9f8)
Flags: in-testsuite?
See Also: → 1402124
Hits "Assertion failure: false (should have already reflowed the kid), at /builds/worker/workspace/build/src/layout/svg/SVGTextFrame.cpp:5171" on debug builds. Testcase asserts back more than a year.
See Also: → 1402486
Like bug 1402486, this also started crashing opt builds after bug 1342951 landed.
Blocks: 1342951
Priority: -- → P3
[Tracking Requested - why for this release]:

This will need fixing and uplifting to avoid shipping an OOBR in 57.
Assignee: nobody → jwatt
Priority: P3 → P1
This testcase puts the SVG text under an element that doesn't render its content tree directly (caption), which means that the layout flushing GetSVGTextFrame() call in SVGTextContentElement::GetStartPositionOfChar doesn't result in an SVGTextFrame::ReflowSVG() call.
The change that caused this problem is this line of the part 3 patch from bug 1342951:

https://hg.mozilla.org/mozilla-central/rev/302d9e49ac75#l3.61

That change made CharIterator use the passed SVGTextFrame and allow iteration regardless of whether it has been reflowed or not, whereas previously it would have acted as if there was nothing to iterate since the SVGTextFrame is never reflowed for this testcase.

That's fine as far as it goes since CharIterator can be used prior to reflow now. However, there are several pieces of code that have checks directly after constructing an CharIterator that trigger early returns if there's nothing to iterate. For some of these pieces of code (such as SVGTextContentElement::GetStartPositionOfChar) these checks essentially act as an implicit check that the SVGTextFrame's anonymous child has been reflowed, implicitly enforcing that invariant for the code. Now that CharIterator no longer enforces that invariant for code like this it is possible for it to break as it has on this bug's testcase.
Bug 1403583 is going to fix this on 57 by backing out the regressing patch.
Depends on: 1403583
Verified fixed for Fx57 by bug 1403583.
Tracking 58+ for this bug based on Comment 3.
Has Regression Range: --- → yes
Duplicate of this bug: 1402124
Duplicate of this bug: 1402486
As noted in the commit comment, while I would have liked to enhance these methods to handle this situation using SVGTextFrame::ReflowSVGForNonDisplay, that has issues. This patch essentially reverts unintended change in behavior for these methods caused by bug 1342951. In other words, whereas prior to that bug these functions were made no-ops by the checks that were made after constructing the CharIterator, the fix for this bug now makes the "is reflowed" check up front.

SVGTextFrame::GetCharNumAtPosition is worth special mention since it's not immediately obvious that it's using CharIterator. The CharIterator is created under the function that it calls at the end though (ConvertTextElementCharIndexToAddressableIndex). Since it isn't valid to use a TextRenderedRunIterator when reflow didn't happen, it makes sense to put the check and early return at the top of GetCharNumAtPosition though.

Note also that at least two MOZ_ASSERT's will fire under the CharIterator constructor in debug builds, preventing us reaching the InvalidArrayIndex error in debug builds. (Hence why I didn't add another MOZ_ASSERT to ConvertTextElementCharIndexToAddressableIndex, since there are MOZ_ASSERT's deeper in the code where issues actually exist.
Comment on attachment 8917056 [details]
Bug 1402109. Prevent crashes under SVGTextElement methods when the SVG is under non-displayed HTML.

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D107#2470
Attachment #8917056 - Flags: review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7ad98e5be6
Prevent crashes under SVGTextElement methods when the SVG is under non-displayed HTML. r=heycam
NI myself to land a crashtest.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/4d7ad98e5be6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(ryanvm)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.