1589778 - Hit MOZ_CRASH(ElementAt(aIndex = 2, aLength = 2)) in [@ ClusterIterator::NextCluster]

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

This looks like it could be potentially bad so I'll mark it as s-s for now.

Note: The test case seems to have a timing aspect so it may take a few reloads to trigger the issue. Please let me know if a Pernosco session would be helpful and I will create one.

Hit MOZ_CRASH(ElementAt(aIndex = 2, aLength = 2)) at src/xpcom/ds/nsTArray.cpp:29

#0 MOZ_Crash src/obj-firefox/dist/include/mozilla/Assertions.h:332:3
#1 InvalidArrayIndex_CRASH(unsigned long, unsigned long) src/xpcom/ds/nsTArray.cpp:27:3
#2 ElementAt src/obj-firefox/dist/include/nsTArray.h:1067:7
#3 operator[] src/obj-firefox/dist/include/nsTArray.h:1103:53
#4 ClusterIterator::NextCluster() src/layout/generic/nsTextFrame.cpp:7952:9
#5 nsTextFrame::PeekOffsetWord(bool, bool, bool, int*, nsIFrame::PeekWordState*, bool) src/layout/generic/nsTextFrame.cpp:8065:14
#6 nsIFrame::PeekOffset(nsPeekOffsetStruct*) src/layout/generic/nsFrame.cpp:8600:22
#7 nsFrameSelection::MoveCaret(nsDirection, bool, nsSelectionAmount, nsFrameSelection::CaretMovementStyle) src/layout/generic/nsFrameSelection.cpp:756:7
#8 mozilla::dom::Selection::Modify(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) src/dom/base/Selection.cpp:3370:24
#9 mozilla::dom::Selection_Binding::modify(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Selection*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/SelectionBinding.cpp:1075:24
#10 bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3198:13
#11 CallJSNative src/js/src/vm/Interpreter.cpp:458:13
#12 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:550:12
#13 CallFromStack src/js/src/vm/Interpreter.cpp:623:10
#14 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3112:16
#15 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:425:10
#16 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:591:13
#17 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) src/js/src/vm/Interpreter.cpp:636:8
#18 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2722:10
#19 mozilla::dom::FrameRequestCallback::Call(JSContext*, JS::Handle<JS::Value>, double, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/WindowBinding.cpp:984:8
#20 Call src/obj-firefox/dist/include/mozilla/dom/WindowBinding.h:608:12
#21 Call src/obj-firefox/dist/include/mozilla/dom/WindowBinding.h:621:12
#22 nsRefreshDriver::RunFrameRequestCallbacks(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1734:44
#23 nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1968:7
#24 TickDriver src/layout/base/nsRefreshDriver.cpp:373:13
#25 mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:350:7
#26 mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:367:5
#27 RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:807:5
#28 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:727:16
#29 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) src/layout/base/nsRefreshDriver.cpp:622:9
#30 mozilla::layout::VsyncChild::RecvNotify(mozilla::VsyncEvent const&) src/layout/ipc/VsyncChild.cpp:65:16
#31 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:187:54
#32 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:5876:32
#33 mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2187:25
#34 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2111:9
#35 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1954:3
#36 mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1985:13
#37 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1225:14
#38 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
#39 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:110:5
#40 RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
#41 RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
#42 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
#43 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#44 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:934:20
#45 RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
#46 RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
#47 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
#48 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:769:34
#49 content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#50 main src/browser/app/nsBrowserApp.cpp:272:18

Comment 1

[Mats Palmgren (inactive)]

This is easy to reproduce on Linux in rr. It's an out-of-bounds read of mWordBreaks:
https://searchfox.org/mozilla-central/rev/55aa17110091deef24b913d033ccaf58f9c6d337/layout/generic/nsTextFrame.cpp#7774
As the assertion indicates, it has 2 entries and we index it with 2, here:
https://searchfox.org/mozilla-central/rev/55aa17110091deef24b913d033ccaf58f9c6d337/layout/generic/nsTextFrame.cpp#7952
GetBeforeOffset() is 2, GetContentOffset() is 0.
Notably, this ClusterIterator iterates backwards (mDirection is -1).

I haven't looked at this code recently so I'm not sure exactly how it's supposed to work, but perhaps we should simply subtract 1 when mDirection is -1 to get the correct array index for mWordBreaks?

Comment 2

[Mats Palmgren (inactive)]

mWordBreaks is an nsTArray which has bound-checks also in release builds, so this should always result in a safe crash.

Comment 3

[Tyson Smith [:tsmith]]

I am able to reproduce the issue with m-c 20230414-7011cafe7d2c.

ni? me if you'd like a Pernosco session.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Tyson Smith [:tsmith] from comment #3)

ni? me if you'd like a Pernosco session.

That would be helpful, yeah - let's get one if it's not too much trouble.

Comment 5

[Jonathan Kew [:jfkthame]]

+1 to that. I don't immediately see what's wrong with the code here.... the reverse-direction ClusterIterator should indeed start at the end of the text (which contains one supplementary-plane character, so that's two 16-bit code units long), but that should be OK because mWordBreaks is explicitly given textLen + 1 elements here, to allow for access to the after-the-last-character position.

So I guess something else is going on here; maybe that's not the fragment of text it's looking at.

Ohhh.... wait a sec, I have an idea. I think this might be related to the short-circuit code path we take when creating textruns for zero-sized fonts (which may or may not really be needed any more; comments refer to old Apple and Windows font APIs). I'll poke at this a bit (unless you get to it first...)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1589778 - Ensure cluster boundaries are set up even in blank textruns. r=#layout-reviewers

Comment 7

[Jonathan Kew [:jfkthame]]

We should be able to adapt the testcase here as a crashtest, too -- it rarely if ever seems to crash for me as it stands, but if I do a Select-All after loading it, that gives me a reliable crash.

Comment 8

[Daniel Holbert [:dholbert]]

Let's repurpose the needinfo=Tyson from "yes please pernosco" to instead "could you confirm that jfkthame's patch fixes the bug". :)

(Sounds like it probably does fix the bug, given jfkthame's local testing in comment 7.)

Comment 9

[Daniel Holbert [:dholbert]]

[er, didn't mean to clear ni; see comment 8.]

Comment 10

[Tyson Smith [:tsmith]]

I'm unable to reproduce the issue with or without the patch when I build locally.

Any tips?

Comment 11

[Tyson Smith [:tsmith]]

Sorry I take that back, the patch does resolve the issue.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1589778 - Add crashtest. r=#layout-reviewers

Comment 13

[Jonathan Kew [:jfkthame]]

The slightly modified testcase here crashes reliably for me when run as a crashtest (without the fix), and loads fine once the patch is applied.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1f8bcf4f681

The only crashes I see with this signature in Socorro look like the result of Jonathan's crashtest testing, so I don't think we need to worry about uplifting this :-)