1472328 - Assertion failure: aPos->GetOriginalOffset() < aOriginalEnd (character outside string), at /builds/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:3290

Status: VERIFIED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20180628100518
SourceStamp=b429b9fb68f1a954c4a9f8dba8e845cf7f569a56

Assertion failure: aPos->GetOriginalOffset() < aOriginalEnd (character outside string), at src/layout/generic/nsTextFrame.cpp:3290

#0 FindClusterEnd(gfxTextRun const*, int, gfxSkipCharsIterator*, bool) src/layout/generic/nsTextFrame.cpp:3289:3
#1 nsTextFrame::GetCharacterRectsInRange(int, int, nsTArray<nsRect>&) src/layout/generic/nsTextFrame.cpp:7825:9
#2 mozilla::ContentEventHandler::OnQueryTextRectArray(mozilla::WidgetQueryContentEvent*) src/dom/events/ContentEventHandler.cpp:2010:24
#3 mozilla::IMEContentObserver::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*) src/dom/events/IMEContentObserver.cpp:826:25
#4 mozilla::EventStateManager::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*) src/dom/events/EventStateManager.cpp:971:22
#5 mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, nsIContent*) src/dom/events/EventStateManager.cpp:544:5
#6 mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) src/layout/base/PresShell.cpp:7607:19
#7 mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) src/layout/base/PresShell.cpp:7335:12
#8 nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) src/view/nsViewManager.cpp:812:14
#9 nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) src/view/nsView.cpp:1141:9
#10 mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) src/widget/PuppetWidget.cpp:413:35
#11 mozilla::ContentCacheInChild::QueryCharRectArray(nsIWidget*, unsigned int, unsigned int, nsTArray<mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel> >&) const src/widget/ContentCache.cpp:314:12
#12 mozilla::ContentCacheInChild::CacheTextRects(nsIWidget*, mozilla::widget::IMENotification const*) src/widget/ContentCache.cpp:379:9
#13 mozilla::ContentCacheInChild::CacheSelection(nsIWidget*, mozilla::widget::IMENotification const*) src/widget/ContentCache.cpp:187:10
#14 mozilla::ContentCacheInChild::CacheText(nsIWidget*, mozilla::widget::IMENotification const*) src/widget/ContentCache.cpp:276:10
#15 mozilla::ContentCacheInChild::CacheAll(nsIWidget*, mozilla::widget::IMENotification const*) src/widget/ContentCache.cpp:148:7
#16 mozilla::widget::PuppetWidget::NotifyIMEOfFocusChange(mozilla::widget::IMENotification const&) src/widget/PuppetWidget.cpp:820:11
#17 mozilla::widget::TextEventDispatcher::NotifyIME(mozilla::widget::IMENotification const&) src/widget/TextEventDispatcher.cpp:494:27
#18 nsBaseWidget::NotifyIME(mozilla::widget::IMENotification const&) src/widget/nsBaseWidget.cpp:1918:43
#19 mozilla::IMEStateManager::NotifyIME(mozilla::widget::IMENotification const&, nsIWidget*, mozilla::dom::TabParent*) src/dom/events/IMEStateManager.cpp:1692:22
#20 mozilla::IMEContentObserver::IMENotificationSender::SendFocusSet() src/dom/events/IMEContentObserver.cpp:1978:3
#21 mozilla::IMEContentObserver::IMENotificationSender::Run() src/dom/events/IMEContentObserver.cpp:1858:5
#22 mozilla::IMEContentObserver::TryToFlushPendingNotifications(bool) src/dom/events/IMEContentObserver.cpp:1725:17
#23 mozilla::EditorEventListener::Focus(mozilla::InternalFocusEvent*) src/editor/libeditor/EditorEventListener.cpp:1132:3
#24 mozilla::EditorEventListener::HandleEvent(mozilla::dom::Event*) src/editor/libeditor/EditorEventListener.cpp:462:14
#25 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1124:52
#26 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1298:20
#27 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:408:17
#28 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:552:12
#29 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1088:9
#30 FocusBlurEvent::Run() src/dom/base/nsFocusManager.cpp:2062:12
#31 nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>) src/dom/base/nsContentUtils.cpp:5724:13
#32 nsContentUtils::AddScriptRunner(nsIRunnable*) src/dom/base/nsContentUtils.cpp:5731:3
#33 nsFocusManager::FireFocusOrBlurEvent(mozilla::EventMessage, nsIPresShell*, nsISupports*, bool, bool, mozilla::dom::EventTarget*) src/dom/base/nsFocusManager.cpp:2235:5
#34 nsFocusManager::SendFocusOrBlurEvent(mozilla::EventMessage, nsIPresShell*, nsIDocument*, nsISupports*, unsigned int, bool, bool, mozilla::dom::EventTarget*) src/dom/base/nsFocusManager.cpp:2200:3
#35 nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, nsIContent*) src/dom/base/nsFocusManager.cpp:1981:7
#36 nsFocusManager::WindowRaised(mozIDOMWindowProxy*) src/dom/base/nsFocusManager.cpp:749:3
#37 nsWebBrowser::Activate() src/toolkit/components/browser/nsWebBrowser.cpp:1817:16
#38 mozilla::dom::TabChild::RecvActivate() src/dom/ipc/TabChild.cpp:1487:12
#39 mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PContentChild.cpp:8391:20
#40 mozilla::dom::ContentChild::OnMessageReceived(IPC::Message const&) src/dom/ipc/ContentChild.cpp:3806:25
#41 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2134:25
#42 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2064:17
#43 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1910:5
#44 mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1943:15
#45 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1051:14
#46 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#47 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#48 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#49 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#50 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
#51 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:896:22
#52 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#53 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#54 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#55 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:722:34
#56 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#57 main src/browser/app/nsBrowserApp.cpp:287:18
#58 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#59 _start (firefox+0x4236e4)

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

It's either potentially accessing beyond array boundary, or the assertion is wrong, I suppose.

Comment 2

[Tyson Smith [:tsmith]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)

It's either potentially accessing beyond array boundary, or the assertion is
wrong, I suppose.

That sounds potentially bad. Let's marks this s-s until it is investigated.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you help find an owner to investigate this (potentially) sec-high issue? Thanks!

Comment 4

[Jonathan Kew [:jfkthame]]

It looks like GetCharacterRectsInRange wants to extend the given range to cluster boundaries; but in this testcase, we have the (unusual) situation that the textframe's content ends mid-cluster. It's possible we should ideally be keeping the 0A71 here, as combining mark, "glued" to the preceding character (>), and not allowing a column break to be forced between them, but I think we may be losing track of that thanks to the script/font boundary that intervenes.

In any case, though, we can make GetCharacterRectsInRange safer and avoid hitting the assertion here by simply testing whether we're already at the end of the content before we try to look ahead for a cluster boundary.

FWIW, I'm not sure there really is much of a security issue here; in this situation, FindClusterEnd will return after advancing a single position, and then GetCharacterRectsInRange will return a rect based on the advance width to this offset (whether or not it really is a cluster end); it won't be accessing beyond the end of the textrun, AFAICS, because there was more text present, it just wasn't part of the current frame's content. So it's possible we might highlight a slightly incorrect area, for example, but it's hard to see much worse happening.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1472328 - Check if we're already at the end of the frame's content. r=mats

Comment 6

[Maire Reavy [:mreavy]]

Hey Dan, referring to comment 4 above from Jonathan, should we downgrade this?

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 9092807 [details]
Bug 1472328 - Check if we're already at the end of the frame's content. r=mats

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't think this is readily exploitable, afaict; it could perhaps result in slightly incorrect rendering/UX, but otherwise looks safe to me. (See comment 4.)
  (I think we could downgrade the security rating, but to be on the safe side, haven't done so in case I'm overlooking some way this could be exploited further.)
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should transplant trivially
• How likely is this patch to cause regressions; how much testing does it need?: Minimal risk

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9092807 [details]
Bug 1472328 - Check if we're already at the end of the frame's content. r=mats

sec-approval+ for mozilla-central.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/cc64c7f364b796a1696a7012822c3b7af7dfea15
https://hg.mozilla.org/mozilla-central/rev/cc64c7f364b7

Comment 10

[Simona Badau, Desktop QA]

I tried to verify this issue, but I can't reproduce the initial Assertion failure. I tested on a Windows 10 x64 machine and on Ubuntu machines (16.04 and 18.04) using asan and mc builds from the 29th of June 2018.

Jonathan, could you please give me some additional information that could help me reproduce this?

1. Is this issue OS dependent? On what OS should I reproduce/verify this?
2. Is this issue reproducible only on certain builds?
3. Is this issue still reproducible with the provided test case from the Description? If yes, are there any additional steps needed and not provided in the Description, that could help me in reproducing this issue?

Comment 11

[Jonathan Kew [:jfkthame]]

You probably need to use a Debug build to reproduce this; it's a Debug-only assertion, and wouldn't necessarily trigger an error in an Opt/Asan build.

Comment 12

[Simona Badau, Desktop QA]

(In reply to Jonathan Kew (:jfkthame) from comment #11)

You probably need to use a Debug build to reproduce this; it's a Debug-only assertion, and wouldn't necessarily trigger an error in an Opt/Asan build.

Thanks, Jonathan! Managed to reproduce this issue on a Firefox 70.0.2 debug build on Ubuntu 18.04 x64. Verified as fixed on the latest Firefox 71 beta 12 debug build on Ubuntu 18.04 x64.