Closed Bug 1295354 Opened 4 years ago Closed 4 years ago

MOZ_ASSERT(nodePosition.mOffset) on e10s while sending a message with an emoticon on hangouts.google.com

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: emilio, Assigned: masayuki)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

I hit this on current nightly, and it causes my computer to freeze due to a OOM. I grabbed a debug build and hit this assertion.

STR:

  1 - Open hangouts.google.com, and log in (sorry!).
  2 - Open an existing text conversation, and type a message with an emoticon, such as |:)|.
  3 - See the content process freeze as it takes more and more memory.

It's not reproducible a 100% of the time, but it's pretty frequently (50+% of them). I've only being able to reproduce it in conversations that were inactive for a few minutes (where the avatar of the other person is at the bottom of the conversation, not a message of yours).

Nightly crash (killed manually before OOM): https://crash-stats.mozilla.com/report/index/97a3772d-5d69-4d46-9a35-bf1fa2160815

Linux GTK, just in case it's widget-dependent.

Backtrace in a debug build:

#6  0x00007f9d407b5051 in mozilla::ContentEventHandler::GetLastFrameInRangeForTextRect (this=0x7ffeb40a6af0, aRange=0x7f9d031e0a00)
    at /home/emilio/projects/moz/gecko-4/dom/events/ContentEventHandler.cpp:1618
1618	    MOZ_ASSERT(nodePosition.mOffset);
(gdb) Quit
(gdb) bt
#0  0x00007f9d3bd32ffd in nanosleep () at /usr/lib/libc.so.6
#1  0x00007f9d3bd32f4a in sleep () at /usr/lib/libc.so.6
#2  0x00007f9d425a6b34 in ah_crap_handler(int) (signum=11) at /home/emilio/projects/moz/gecko-4/toolkit/xre/nsSigHandlers.cpp:103
#3  0x00007f9d425a6b7f in child_ah_crap_handler(int) (signum=11) at /home/emilio/projects/moz/gecko-4/toolkit/xre/nsSigHandlers.cpp:115
#4  0x00007f9d43757fea in AsmJSFaultHandler<(Signal)0>(int, siginfo_t*, void*) (signum=11, info=0x7ffeb40a5c30, context=0x7ffeb40a5b00)
    at /home/emilio/projects/moz/gecko-4/js/src/asmjs/WasmSignalHandlers.cpp:1211
#5  0x00007f9d4759c080 in <signal handler called> () at /usr/lib/libpthread.so.0
#6  0x00007f9d407b5051 in mozilla::ContentEventHandler::GetLastFrameInRangeForTextRect(nsRange*) (this=0x7ffeb40a6af0, aRange=0x7f9d031e0a00)
    at /home/emilio/projects/moz/gecko-4/dom/events/ContentEventHandler.cpp:1618
#7  0x00007f9d407b7151 in mozilla::ContentEventHandler::OnQueryTextRect(mozilla::WidgetQueryContentEvent*) (this=0x7ffeb40a6af0, aEvent=0x7ffeb40a67a0)
    at /home/emilio/projects/moz/gecko-4/dom/events/ContentEventHandler.cpp:2143
#8  0x00007f9d407b6ac7 in mozilla::ContentEventHandler::OnQueryTextRectArray(mozilla::WidgetQueryContentEvent*) (this=0x7ffeb40a6af0, aEvent=0x7ffeb40a79b0)
    at /home/emilio/projects/moz/gecko-4/dom/events/ContentEventHandler.cpp:2067
#9  0x00007f9d407b385f in mozilla::ContentEventHandler::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*) (this=0x7ffeb40a6af0, aEvent=0x7ffeb40a79b0)
    at /home/emilio/projects/moz/gecko-4/dom/events/ContentEventHandler.cpp:1248
#10 0x00007f9d407e6103 in mozilla::IMEContentObserver::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*) (this=0x7f9d03692bc0, aEvent=0x7ffeb40a79b0)
    at /home/emilio/projects/moz/gecko-4/dom/events/IMEContentObserver.cpp:775
#11 0x00007f9d40798436 in mozilla::EventStateManager::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*) (this=0x7f9d0b8b8540, aEvent=0x7ffeb40a79b0)
    at /home/emilio/projects/moz/gecko-4/dom/events/EventStateManager.cpp:871
#12 0x00007f9d40797b3e in mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*) (this=0x7f9d0b8b8540, aPresContext=0x7f9d05bca800, aEvent=0x7ffeb40a79b0, aTargetFrame=0x7f9d0b70e768, aTargetContent=0x7f9d11d53ca0, aStatus=0x7ffeb40a78cc) at /home/emilio/projects/moz/gecko-4/dom/events/EventStateManager.cpp:595
#13 0x00007f9d419f2d8f in PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) (this=0x7f9d04a3c000, aEvent=0x7ffeb40a79b0, aStatus=0x7ffeb40a78cc, aIsHandlingNativeEvent=true) at /home/emilio/projects/moz/gecko-4/layout/base/nsPresShell.cpp:8495
#14 0x00007f9d419f218f in PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) (this=0x7f9d04a3c000, aFrame=0x7f9d05bf8968, aEvent=0x7ffeb40a79b0, aDontRetargetEvents=true, aEventStatus=0x7ffeb40a78cc, aTargetContent=0x0) at /home/emilio/projects/moz/gecko-4/layout/base/nsPresShell.cpp:8220
#15 0x00007f9d419f036b in PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) (this=0x7f9d116d6800, aFrame=0x7f9d11b14b68, aEvent=0x7ffeb40a79b0, aDontRetargetEvents=false, aEventStatus=0x7ffeb40a78cc, aTargetContent=0x0) at /home/emilio/projects/moz/gecko-4/layout/base/nsPresShell.cpp:7723
#16 0x00007f9d41502d31 in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) (this=0x7f9d0b2c4ac0, aEvent=0x7ffeb40a79b0, aView=0x7f9d03c12980, aStatus=0x7ffeb40a78cc) at /home/emilio/projects/moz/gecko-4/view/nsViewManager.cpp:816
#17 0x00007f9d414ff270 in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) (this=0x7f9d03c12980, aEvent=0x7ffeb40a79b0, aUseAttachedEvents=false)
    at /home/emilio/projects/moz/gecko-4/view/nsView.cpp:1121
#18 0x00007f9d4152f7ca in mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) (this=0x7f9d1d64f400, event=0x7ffeb40a79b0, aStatus=@0x7ffeb40a7acc: nsEventStatus_eIgnore) at /home/emilio/projects/moz/gecko-4/widget/PuppetWidget.cpp:350
#19 0x00007f9d4151d3ea in mozilla::ContentCacheInChild::QueryCharRectArray(nsIWidget*, unsigned int, unsigned int, nsTArray<mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel> >&) const (this=0x7f9d1d64f5e8, aWidget=0x7f9d1d64f400, aOffset=0, aLength=2, aCharRectArray=...) at /home/emilio/projects/moz/gecko-4/widget/ContentCache.cpp:309
#20 0x00007f9d4151d8ab in mozilla::ContentCacheInChild::CacheTextRects(nsIWidget*, mozilla::widget::IMENotification const*) (this=0x7f9d1d64f5e8, aWidget=0x7f9d1d64f400, aNotification=0x7ffeb40a8400) at /home/emilio/projects/moz/gecko-4/widget/ContentCache.cpp:371
#21 0x00007f9d4151c946 in mozilla::ContentCacheInChild::CacheSelection(nsIWidget*, mozilla::widget::IMENotification const*) (this=0x7f9d1d64f5e8, aWidget=0x7f9d1d64f400, aNotification=0x7ffeb40a8400) at /home/emilio/projects/moz/gecko-4/widget/ContentCache.cpp:182
#22 0x00007f9d4151d17d in mozilla::ContentCacheInChild::CacheText(nsIWidget*, mozilla::widget::IMENotification const*) (this=0x7f9d1d64f5e8, aWidget=0x7f9d1d64f400, aNotification=0x7ffeb40a8400) at /home/emilio/projects/moz/gecko-4/widget/ContentCache.cpp:271
#23 0x00007f9d41530b71 in mozilla::widget::PuppetWidget::NotifyIMEOfTextChange(mozilla::widget::IMENotification const&) (this=0x7f9d1d64f400, aIMENotification=...)
    at /home/emilio/projects/moz/gecko-4/widget/PuppetWidget.cpp:850
#24 0x00007f9d415304bd in mozilla::widget::PuppetWidget::NotifyIMEInternal(mozilla::widget::IMENotification const&) (this=0x7f9d1d64f400, aIMENotification=...)
    at /home/emilio/projects/moz/gecko-4/widget/PuppetWidget.cpp:639
#25 0x00007f9d4150f713 in nsBaseWidget::NotifyIME(mozilla::widget::IMENotification const&) (this=0x7f9d1d64f400, aIMENotification=...)
    at /home/emilio/projects/moz/gecko-4/widget/nsBaseWidget.cpp:1854
#26 0x00007f9d407eeb9d in mozilla::IMEStateManager::NotifyIME(mozilla::widget::IMENotification const&, nsIWidget*, bool) (aNotification=..., aWidget=0x7f9d1d64f400, aOriginIsRemote=false)
    at /home/emilio/projects/moz/gecko-4/dom/events/IMEStateManager.cpp:1426
---Type <return> to continue, or q <return> to quit---
#27 0x00007f9d407e9a05 in mozilla::IMEContentObserver::IMENotificationSender::SendTextChange() (this=0x7f9d0328be20)
    at /home/emilio/projects/moz/gecko-4/dom/events/IMEContentObserver.cpp:1769
#28 0x00007f9d407e8bc3 in mozilla::IMEContentObserver::IMENotificationSender::Run() (this=0x7f9d0328be20) at /home/emilio/projects/moz/gecko-4/dom/events/IMEContentObserver.cpp:1556
#29 0x00007f9d407e85d3 in mozilla::IMEContentObserver::TryToFlushPendingNotifications() (this=0x7f9d03692bc0) at /home/emilio/projects/moz/gecko-4/dom/events/IMEContentObserver.cpp:1437
#30 0x00007f9d40797454 in mozilla::EventStateManager::TryToFlushPendingNotificationsToIME() (this=0x7f9d0b8b8540) at /home/emilio/projects/moz/gecko-4/dom/events/EventStateManager.cpp:479
#31 0x00007f9d419f2fa5 in PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) (this=0x7f9d04a3c000, aEvent=0x7ffeb40a9160, aStatus=0x7ffeb40a904c, aIsHandlingNativeEvent=true) at /home/emilio/projects/moz/gecko-4/layout/base/nsPresShell.cpp:8530
#32 0x00007f9d419f218f in PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) (this=0x7f9d04a3c000, aFrame=0x7f9d05bf8968, aEvent=0x7ffeb40a9160, aDontRetargetEvents=true, aEventStatus=0x7ffeb40a904c, aTargetContent=0x0) at /home/emilio/projects/moz/gecko-4/layout/base/nsPresShell.cpp:8220
#33 0x00007f9d419f036b in PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) (this=0x7f9d116d6800, aFrame=0x7f9d11b14b68, aEvent=0x7ffeb40a9160, aDontRetargetEvents=false, aEventStatus=0x7ffeb40a904c, aTargetContent=0x0) at /home/emilio/projects/moz/gecko-4/layout/base/nsPresShell.cpp:7723
#34 0x00007f9d41502d31 in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) (this=0x7f9d0b2c4ac0, aEvent=0x7ffeb40a9160, aView=0x7f9d03c12980, aStatus=0x7ffeb40a904c) at /home/emilio/projects/moz/gecko-4/view/nsViewManager.cpp:816
#35 0x00007f9d414ff270 in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) (this=0x7f9d03c12980, aEvent=0x7ffeb40a9160, aUseAttachedEvents=false)
    at /home/emilio/projects/moz/gecko-4/view/nsView.cpp:1121
#36 0x00007f9d4152f7ca in mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) (this=0x7f9d1d64f400, event=0x7ffeb40a9160, aStatus=@0x7ffeb40a912c: nsEventStatus_eIgnore) at /home/emilio/projects/moz/gecko-4/widget/PuppetWidget.cpp:350
#37 0x00007f9d3f0f4e1f in mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) (aEvent=...)
    at /home/emilio/projects/moz/gecko-4/gfx/layers/apz/util/APZCCallbackHelper.cpp:463
#38 0x00007f9d4117c78e in mozilla::dom::TabChild::RecvRealKeyEvent(mozilla::WidgetKeyboardEvent const&, mozilla::dom::MaybeNativeKeyBinding const&) (this=0x7f9d1d64ec00, event=..., aBindings=...) at /home/emilio/projects/moz/gecko-4/dom/ipc/TabChild.cpp:2107
#39 0x00007f9d3e861ae9 in mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) (this=0x7f9d1d64ec60, msg__=...)
    at /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/ipc/ipdl/PBrowserChild.cpp:3897
#40 0x00007f9d3e96d637 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (this=0x7f9d2fc81020, msg__=...)
    at /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentChild.cpp:7438
#41 0x00007f9d3e28bbbc in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (this=0x7f9d2fc81088, aMsg=...)
    at /home/emilio/projects/moz/gecko-4/ipc/glue/MessageChannel.cpp:1662
#42 0x00007f9d3e28b6c0 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=0x7f9d2fc81088, aMsg=<unknown type in /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so, CU 0x2167172, DIE 0x21e3b49>) at /home/emilio/projects/moz/gecko-4/ipc/glue/MessageChannel.cpp:1600
#43 0x00007f9d3e28b40d in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (this=0x7f9d2fc81088) at /home/emilio/projects/moz/gecko-4/ipc/glue/MessageChannel.cpp:1567
#44 0x00007f9d3e2a8e28 in mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (o=0x7f9d2fc81088, m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f9d3e28b2ac <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, args=...) at /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:729
#45 0x00007f9d3e2a8ad8 in mozilla::detail::RunnableMethodArguments<>::apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)()) (this=0x7f9d2fc41b38, o=0x7f9d2fc81088, m=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7f9d3e28b2ac <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>) at /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:736
#46 0x00007f9d3e2a85bd in mozilla::detail::RunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() (this=0x7f9d2fc41b00)
    at /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:764
#47 0x00007f9d3e297075 in mozilla::ipc::MessageChannel::RefCountedTask::Run() (this=0x7f9d2fc4b6d0)
    at /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/MessageChannel.h:545
#48 0x00007f9d3e297284 in mozilla::ipc::MessageChannel::DequeueTask::Run() (this=0x7f9d0315f7f0)
    at /home/emilio/projects/moz/gecko-4/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/MessageChannel.h:564
#49 0x00007f9d3daf6e30 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f9d2eccc100, aMayWait=true, aResult=0x7ffeb40acc4f)
    at /home/emilio/projects/moz/gecko-4/xpcom/threads/nsThread.cpp:1058
#50 0x00007f9d3db5fe50 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7f9d2eccc100, aMayWait=true) at /home/emilio/projects/moz/gecko-4/xpcom/glue/nsThreadUtils.cpp:290
#51 0x00007f9d3e28f95a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f9d2fc83290, aDelegate=0x7ffeb40acee0)
    at /home/emilio/projects/moz/gecko-4/ipc/glue/MessagePump.cpp:124
#52 0x00007f9d3e290268 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f9d2fc83290, aDelegate=0x7ffeb40acee0)
    at /home/emilio/projects/moz/gecko-4/ipc/glue/MessagePump.cpp:301
---Type <return> to continue, or q <return> to quit---
#53 0x00007f9d3e1ff58f in MessageLoop::RunInternal() (this=0x7ffeb40acee0) at /home/emilio/projects/moz/gecko-4/ipc/chromium/src/base/message_loop.cc:232
#54 0x00007f9d3e1ff522 in MessageLoop::RunHandler() (this=0x7ffeb40acee0) at /home/emilio/projects/moz/gecko-4/ipc/chromium/src/base/message_loop.cc:225
#55 0x00007f9d3e1ff4fb in MessageLoop::Run() (this=0x7ffeb40acee0) at /home/emilio/projects/moz/gecko-4/ipc/chromium/src/base/message_loop.cc:205
#56 0x00007f9d41543460 in nsBaseAppShell::Run() (this=0x7f9d2253feb0) at /home/emilio/projects/moz/gecko-4/widget/nsBaseAppShell.cpp:156
#57 0x00007f9d425a26b2 in XRE_RunAppShell() () at /home/emilio/projects/moz/gecko-4/toolkit/xre/nsEmbedFunctions.cpp:846
#58 0x00007f9d3e2900fd in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f9d2fc83290, aDelegate=0x7ffeb40acee0)
    at /home/emilio/projects/moz/gecko-4/ipc/glue/MessagePump.cpp:269
#59 0x00007f9d3e1ff58f in MessageLoop::RunInternal() (this=0x7ffeb40acee0) at /home/emilio/projects/moz/gecko-4/ipc/chromium/src/base/message_loop.cc:232
#60 0x00007f9d3e1ff522 in MessageLoop::RunHandler() (this=0x7ffeb40acee0) at /home/emilio/projects/moz/gecko-4/ipc/chromium/src/base/message_loop.cc:225
#61 0x00007f9d3e1ff4fb in MessageLoop::Run() (this=0x7ffeb40acee0) at /home/emilio/projects/moz/gecko-4/ipc/chromium/src/base/message_loop.cc:205
#62 0x00007f9d425a212a in XRE_InitChildProcess(int, char**, XREChildData const*) (aArgc=3, aArgv=0x7ffeb40ad358, aChildData=0x7ffeb40ad200)
    at /home/emilio/projects/moz/gecko-4/toolkit/xre/nsEmbedFunctions.cpp:676
#63 0x00000000004234cf in content_process_main(int, char**) (argc=5, argv=0x7ffeb40ad358) at /home/emilio/projects/moz/gecko-4/ipc/app/../contentproc/plugin-container.cpp:197
#64 0x0000000000423598 in main(int, char**) (argc=6, argv=0x7ffeb40ad358) at /home/emilio/projects/moz/gecko-4/ipc/app/MozillaRuntimeMain.cpp:18


I haven't investigated that much on why this assertion lead to OOM, probably due to the underflowed offset. Let me know if I can provide more info.
Needinfoing Masayuki Nakano, that was according to the git blame log the one who introduced this code a few days ago.
Flags: needinfo?(masayuki)
Thank you for the quick report. Actually, this is a mistake of bug 1286464.
Assignee: nobody → masayuki
Blocks: 1286464
Status: NEW → ASSIGNED
Component: DOM: Events → Event Handling
Flags: needinfo?(masayuki)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Hmm, I cannot reproduce this bug with creating tests. Could you provide the DOM tree's screenshot when you type ":)"?
Hey, sorry for the delay replying, I can still grab a content tree snapshot if you need it.

Just checked it and yeah, I can't reproduce the bug anymore with that build, thanks!
Flags: needinfo?(ealvarez)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Hey, sorry for the delay replying, I can still grab a content tree snapshot
> if you need it.

If you can, I'd like to include the case into automated tests if it's possible. Could you attach a screenshot of Inspector (Ctrl+Shift+C)? Only in the editor is enough.

# As far as I investigated, I have no idea how to reproduce it intentionally. It might be related to empty text node or something...

> Just checked it and yeah, I can't reproduce the bug anymore with that build,
> thanks!

Thank you! If you cannot attach screenshot soon, I'll request reviews for the patches. Let me know if so.
Ah, bad timing. I got reply from my friend by google hangouts. So, I got the DOM tree now.

Although, the DOM tree is not special. It has only a text node created by Gecko, sigh...

Anyway, thank you for your report and test! I'll request review.
Heh, was just doing it, ok then, good luck! :)
FYI: I'm still not sure how to reproduce this bug because this must be tested by the automated tests...

Anyway, I found bugs of the method and its fix causes setting node offset to 0. Therefore, it needs to check the offset before decrementing it.
Comment on attachment 8781512 [details]
Bug 1295354 part.1 ContentEventHandler::GetLastFrameInRangeForTextRect() shouldn't set nextNodeOfRangeEnd to the start node of aRange because the start node should always be in the range

https://reviewboard.mozilla.org/r/71920/#review69434
Attachment #8781512 - Flags: review?(bugs) → review+
Comment on attachment 8781513 [details]
Bug 1295354 part.2 ContentEventHandler::GetLastFrameInRangeForTextRect() shouldn't use empty text node as far as possible

https://reviewboard.mozilla.org/r/71922/#review69446

::: dom/events/ContentEventHandler.cpp:1584
(Diff revision 1)
>        if (node == aRange->GetEndParent()) {
>          nodePosition.mOffset = aRange->EndOffset();
>        } else {
>          nodePosition.mOffset = node->Length();
>        }
> +      // If the text node is empty, we should store current position but

If we called nodePosition.mOffset = aRange->EndOffset();, text node might not be empty here.
So at least the comment needs to be fixed.
Comment on attachment 8781513 [details]
Bug 1295354 part.2 ContentEventHandler::GetLastFrameInRangeForTextRect() shouldn't use empty text node as far as possible

https://reviewboard.mozilla.org/r/71922/#review69448
Attachment #8781513 - Flags: review?(bugs) → review+
Comment on attachment 8781513 [details]
Bug 1295354 part.2 ContentEventHandler::GetLastFrameInRangeForTextRect() shouldn't use empty text node as far as possible

https://reviewboard.mozilla.org/r/71922/#review69446

> If we called nodePosition.mOffset = aRange->EndOffset();, text node might not be empty here.
> So at least the comment needs to be fixed.

New comment:

>       // If the text node is empty or the last node of the range but the index
>       // is 0, we should store current position but continue looking for
>       // previous node (If there are no nodes before it, we should use current
>       // node position for returning its frame).
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/31c1f6df66d2
part.1 ContentEventHandler::GetLastFrameInRangeForTextRect() shouldn't set nextNodeOfRangeEnd to the start node of aRange because the start node should always be in the range r=smaug
https://hg.mozilla.org/integration/autoland/rev/785aa98cb54c
part.2 ContentEventHandler::GetLastFrameInRangeForTextRect() shouldn't use empty text node as far as possible r=smaug
https://hg.mozilla.org/mozilla-central/rev/31c1f6df66d2
https://hg.mozilla.org/mozilla-central/rev/785aa98cb54c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.