767070 - Text selection performance is bad on android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I tested this out on a Nexus S on this page: http://people.mozilla.com/~jmuizelaar/

Profile coming...

Comment 1

[Aaron Train [:aaronmt]]

Excessive screenshots symptom? Faster with it disabled?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to Aaron Train [:aaronmt] from comment #1)
> Excessive screenshots symptom? Faster with it disabled?

Perhaps but I don't see screenshots showing in the profiles.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Here's the profile:
 http://people.mozilla.com/~bgirard/cleopatra/?report=AMIfv94qINFsDl8v4YsWAhVjKSVfyutKyttETseFkUQ-p-JpIXKJBVkLbJwrkC1nVEtDHFNRXA85GzgWlX6t-dnzNK6bDcOPhyX6Wc4B5g__AtjiIWJwLEC46p2hkzeOKsimebHkWSG0kzvya1NoQOdhVhNIDpcHaA

Comment 4

[:Margaret Leibovic]

Jeff, do you know who would be able to help us out with this? Also, is this the same issue causing bug 772140?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Aaron and Martijn: I find that dragging the handles works pretty well on some sites and horrible on others. A simple example: nytimes.com is horrible and mobile.nytimes.com is pretty good.

I have not found and reason for the difference. Can we start with a nytimes.com like test page and start removing stuff (getting it more like mobile.nytimes.com) and see at what point the performance gets better?

Comment 7

[:Margaret Leibovic]

I just uplifted text selection to aurora (bug 695173 and a ton of follow-ups). However, if this issue isn't sorted out and a fix uplifted as well, we'll want to consider disabling this feature before shipping.

Note: I feel like we need to clarify the difference between this bug and bug 772140. Bug 772140 is about text selection being so unresponsive that it's unusable. This bug is more about the slight lag that's present on most pages, and that may be due to the way we process touch events.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Margaret Leibovic [:margaret] from comment #4)
> Jeff, do you know who would be able to help us out with this?

Yeah, I'll take a look with some of the layout people next week.

Comment 9

[Benoit Girard (:BenWa)]

Here is a profile wesj collected. It shows that we're querying position information in browser.js that is causing us to reflow to answer those queries. We need to avoid reflows for position information. Even worse when we do a cycle of query, reflow, update a position and make dirty, query, reflow, update a position and make dirty and repeat:

http://people.mozilla.com/~bgirard/cleopatra/?report=a845f2dc5a23b0757d81779922d2d220ca58fcbd

Comment 10

[Martijn Wargers (dead)]

Attachment: sort of minimized testcase

This is sort of a minimized testcase. Notice the position: absolute; top: -10000px; div in there.

In about:config , turn nglayout.debug.paint_flashing on and watch how the whole text gets repainted every time the absolute positioned div moves.
That should not be happening and is a regression.

Comment 11

[Martijn Wargers (dead)]

A regression range might be useful here.

Comment 12

[Benoit Girard (:BenWa)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #10)
> Created attachment 642031 [details]
> sort of minimized testcase
> 
> This is sort of a minimized testcase. Notice the position: absolute; top:
> -10000px; div in there.
> 
> In about:config , turn nglayout.debug.paint_flashing on and watch how the
> whole text gets repainted every time the absolute positioned div moves.
> That should not be happening and is a regression.

This testcase here is a case of overpainting. I don't think it's related to text selection. This should be a different bug.

Comment 13

[Martijn Wargers (dead)]

The text selection indicators are absolute positioned divs added to the page (by using a -moz-binding on the root element), I think it is comparable to the testcase.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Testing patches

Most of these changes make text-selection not work, but I wanted to throw this up anyway for reference. Performance still isn't very good either (using a nytimes story). Profile at this point:

http://people.mozilla.com/~bgirard/cleopatra/?report=65c53178816b88b60d71036f361949806299bd96

Comment 15

[Wesley Johnston (:wesj)]

I should say that makes text selection not work well. It works, but with a few quirks.

Comment 16

[Wesley Johnston (:wesj)]

Locally I've been breaking stuff to look into this just a bit move. There are still calls to PresShell::FrameNeedsReflow reflows that occur in our current code because of calls to 1.) elementFromPoint and 2.) getClientRects().

Removing those (and breaking text selection in the process), we still reflow though! and on these particular pages that apparently means multisecond pauses. I grabbed a backtrace from this reflows because the profiler didn't have much info. It looks like just sending the mouse event causes this to run?

#0  0x4ae87c74 in PresShell::FrameNeedsReflow(nsIFrame*, nsIPresShell::IntrinsicDirty, unsigned long long) ()
   from /home/wesj/src/android/dist/bin/libxul.so
#1  0x4aeb2c50 in nsGfxScrollFrameInner::UpdateOverflow() () from /home/wesj/src/android/dist/bin/libxul.so
#2  0x4ae965e4 in nsHTMLScrollFrame::UpdateOverflow (this=<optimized out>)
    at /home/wesj/src/mozilla-central/layout/forms/../generic/nsGfxScrollFrame.h:506
#3  0x4ae5a69a in nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&) () from /home/wesj/src/android/dist/bin/libxul.so
#4  0x4ae5a8d6 in nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::css::RestyleTracker&, bool) ()
   from /home/wesj/src/android/dist/bin/libxul.so
#5  0x4ae50a90 in mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) ()
   from /home/wesj/src/android/dist/bin/libxul.so
#6  0x4ae50e26 in mozilla::css::RestyleTracker::DoProcessRestyles() () from /home/wesj/src/android/dist/bin/libxul.so
#7  0x4ae5a842 in nsCSSFrameConstructor::ProcessPendingRestyles() () from /home/wesj/src/android/dist/bin/libxul.so
#8  0x4ae875ea in PresShell::FlushPendingNotifications(mozFlushType) () from /home/wesj/src/android/dist/bin/libxul.so
#9  0x4ae7f450 in nsPresShellEventCB::HandleEvent (this=0x44f32b78, aVisitor=...) at /home/wesj/src/mozilla-central/layout/base/nsPresShell.cpp:472
#10 0x4b00c95c in nsEventTargetChainItem::HandleEventTargetChain (this=<optimized out>, aVisitor=..., aFlags=6, aCallback=0x44f32b78, 
    aMayHaveNewListenerManagers=false, aPusher=0x44f32af8) at /home/wesj/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:362
#11 0x4b00ce30 in nsEventDispatcher::Dispatch (aTarget=<optimized out>, aPresContext=0x4a5b2400, aEvent=0x44f32d08, aDOMEvent=<optimized out>, 
    aEventStatus=0x44f32d80, aCallback=0x44f32b78, aTargets=0x0) at /home/wesj/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:639
#12 0x4ae8620a in PresShell::HandleEventInternal(nsEvent*, nsEventStatus*) () from /home/wesj/src/android/dist/bin/libxul.so
#13 0x4ae8643a in PresShell::HandlePositionedEvent(nsIFrame*, nsGUIEvent*, nsEventStatus*) () from /home/wesj/src/android/dist/bin/libxul.so
#14 0x4ae86f62 in PresShell::HandleEvent(nsIFrame*, nsGUIEvent*, bool, nsEventStatus*) () from /home/wesj/src/android/dist/bin/libxul.so
#15 0x4b0cf97c in nsDOMWindowUtils::SendMouseEventCommon(nsAString_internal const&, float, float, int, int, int, bool, bool) ()
   from /home/wesj/src/android/dist/bin/libxul.so
#16 0x4b0ce5fa in nsDOMWindowUtils::SendMouseEventToWindow(nsAString_internal const&, float, float, int, int, int, bool) ()
   from /home/wesj/src/android/dist/bin/libxul.so
#17 0x4b580824 in NS_InvokeByIndex_P (that=0x4a98cf00, methodIndex=<optimized out>, paramCount=<optimized out>, params=<optimized out>)
    at /home/wesj/src/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:160
#18 0x4b2c7052 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) () from /home/wesj/src/android/dist/bin/libxul.so
#19 0x4b2cbd2a in XPC_WN_CallMethod (cx=0x47684120, argc=7, vp=0x47900228)
    at /home/wesj/src/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
#20 0x4b804fc0 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/wesj/src/android/dist/bin/libxul.so
#21 0x4b805abc in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) () from /home/wesj/src/android/dist/bin/libxul.so
#22 0x4b80c806 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) () from /home/wesj/src/android/dist/bin/libxul.so
#23 0x4b80d81e in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) ()
   from /home/wesj/src/android/dist/bin/libxul.so
#24 0x4b7ae40a in JS_CallFunctionValue (cx=0x47684120, obj=<optimized out>, fval=..., argc=1, argv=0x44f33c28, rval=0x44f33d80)
    at /home/wesj/src/mozilla-central/js/src/jsapi.cpp:5564
#25 0x4b2c42d6 in nsXPCWrappedJSClass::CallMethod (this=0x4a0c3580, wrapper=<optimized out>, methodIndex=<optimized out>, info=0x47605ad0, 
    nativeParams=0x44f33e38) at /home/wesj/src/mozilla-central/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
#26 0x4b2c17c6 in nsXPCWrappedJS::CallMethod (this=0x4e1ab5c0, methodIndex=3, info=0x47605ad0, params=<optimized out>)
    at /home/wesj/src/mozilla-central/js/xpconnect/src/XPCWrappedJS.cpp:580
#27 0x4b58113c in PrepareAndDispatch (self=0x4a913c70, methodIndex=<optimized out>, args=0x44f33ef4)
    at /home/wesj/src/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:105
---Type <return> to continue, or q <return> to quit---
#28 0x4b580874 in SharedStub () from /home/wesj/src/android/dist/bin/libxul.so
#29 0x4aff9314 in nsEventListenerManager::HandleEventSubType (this=<optimized out>, aListenerStruct=<optimized out>, aListener=0x4a913c70, 
    aDOMEvent=0x4cc91180, aCurrentTarget=0x4d382420, aPhaseFlags=6, aPusher=0x44f34040)
    at /home/wesj/src/mozilla-central/content/events/src/nsEventListenerManager.cpp:820
#30 0x4aff9450 in nsEventListenerManager::HandleEventInternal (this=0x4e1f3600, aPresContext=<optimized out>, aEvent=0x44f340b8, aDOMEvent=0x44f34030, 
    aCurrentTarget=0x4d382420, aFlags=6, aEventStatus=0x44f34034, aPusher=0x44f34040)
    at /home/wesj/src/mozilla-central/content/events/src/nsEventListenerManager.cpp:893
#31 0x4b00c7a2 in HandleEvent (aPusher=<optimized out>, aEventStatus=<optimized out>, aFlags=<optimized out>, aCurrentTarget=<optimized out>, 
    aDOMEvent=<optimized out>, aEvent=<optimized out>, aPresContext=<optimized out>, this=<optimized out>)
    at /home/wesj/src/mozilla-central/content/events/src/nsEventListenerManager.h:143
#32 nsEventTargetChainItem::HandleEvent (this=<optimized out>, aVisitor=<optimized out>, aFlags=6, aMayHaveNewListenerManagers=<optimized out>, 
    aPusher=0x44f34040) at /home/wesj/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:184
#33 0x4b00c88c in nsEventTargetChainItem::HandleEventTargetChain (this=<optimized out>, aVisitor=..., aFlags=6, aCallback=0x44f34178, 
    aMayHaveNewListenerManagers=false, aPusher=0x44f34040) at /home/wesj/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:316
#34 0x4b00ce30 in nsEventDispatcher::Dispatch (aTarget=<optimized out>, aPresContext=0x4a5b2400, aEvent=0x44f340b8, aDOMEvent=<optimized out>, 
    aEventStatus=0x44f3410c, aCallback=0x44f34178, aTargets=0x0) at /home/wesj/src/mozilla-central/content/events/src/nsEventDispatcher.cpp:639
#35 0x4ae85bf4 in PresShell::DispatchTouchEvent(nsEvent*, nsEventStatus*, nsPresShellEventCB*, bool) () from /home/wesj/src/android/dist/bin/libxul.so
#36 0x4ae861ee in PresShell::HandleEventInternal(nsEvent*, nsEventStatus*) () from /home/wesj/src/android/dist/bin/libxul.so
#37 0x4ae8643a in PresShell::HandlePositionedEvent(nsIFrame*, nsGUIEvent*, nsEventStatus*) () from /home/wesj/src/android/dist/bin/libxul.so
#38 0x4ae86f62 in PresShell::HandleEvent(nsIFrame*, nsGUIEvent*, bool, nsEventStatus*) () from /home/wesj/src/android/dist/bin/libxul.so
#39 0x4ae866be in PresShell::HandleEvent(nsIFrame*, nsGUIEvent*, bool, nsEventStatus*) () from /home/wesj/src/android/dist/bin/libxul.so
#40 0x4b0cb85e in nsViewManager::DispatchEvent (this=<optimized out>, aEvent=0x44f345d0, aView=0x4a2411c0, aStatus=0x44f344cc)
    at /home/wesj/src/mozilla-central/view/src/nsViewManager.cpp:863
#41 0x4b0c9640 in HandleEvent (aEvent=0x44f345d0) at /home/wesj/src/mozilla-central/view/src/nsView.cpp:127
#42 0x4b433a14 in nsWindow::DispatchEvent(nsGUIEvent*) () from /home/wesj/src/android/dist/bin/libxul.so
#43 0x4b433b3c in nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) () from /home/wesj/src/android/dist/bin/libxul.so
#44 0x4b4348de in nsWindow::DispatchMultitouchEvent(nsTouchEvent&, mozilla::AndroidGeckoEvent*) () from /home/wesj/src/android/dist/bin/libxul.so
#45 0x4b4349c2 in nsWindow::OnMultitouchEvent(mozilla::AndroidGeckoEvent*) () from /home/wesj/src/android/dist/bin/libxul.so
#46 0x4b435f32 in nsWindow::OnGlobalAndroidEvent(mozilla::AndroidGeckoEvent*) () from /home/wesj/src/android/dist/bin/libxul.so
#47 0x4b42a960 in nsAppShell::ProcessNextNativeEvent(bool) () from /home/wesj/src/android/dist/bin/libxul.so
#48 0x4b437b96 in nsBaseAppShell::DoProcessNextNativeEvent(bool, unsigned int) () from /home/wesj/src/android/dist/bin/libxul.so
#49 0x4b437c3a in nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool, unsigned int) () from /home/wesj/src/android/dist/bin/libxul.so
#50 0x4b57497c in nsThread::ProcessNextEvent (this=0x452564c0, mayWait=false, result=0x44f34837)
    at /home/wesj/src/mozilla-central/xpcom/threads/nsThread.cpp:586
#51 0x4b553102 in NS_ProcessNextEvent_P (thread=0x45254590, mayWait=false) at /home/wesj/src/android/xpcom/build/nsThreadUtils.cpp:217
#52 0x4b4b05ee in mozilla::ipc::MessagePump::Run (this=0x452532b0, aDelegate=0x4527d0e0) at /home/wesj/src/mozilla-central/ipc/glue/MessagePump.cpp:82
#53 0x4b596ca4 in MessageLoop::RunInternal (this=0x4ba4c35c) at /home/wesj/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:208
#54 0x4b596d7a in RunHandler (this=<optimized out>) at /home/wesj/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#55 MessageLoop::Run (this=0x4527d0e0) at /home/wesj/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:175
#56 0x4b437624 in nsBaseAppShell::Run() () from /home/wesj/src/android/dist/bin/libxul.so
#57 0x4b3727c4 in nsAppStartup::Run (this=0x475471f0) at /home/wesj/src/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:257
#58 0x4ad6a18a in XREMain::XRE_mainRun (this=0x44f349dc) at /home/wesj/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:3787
#59 0x4ad6c2c0 in XREMain::XRE_main (this=0x44f349dc, argc=<optimized out>, argv=0x4526b048, aAppData=<optimized out>)
---Type <return> to continue, or q <return> to quit---
    at /home/wesj/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:3864
#60 0x4ad6c3f4 in XRE_main (argc=7, argv=0x4526b048, aAppData=0x814211d4, aFlags=<optimized out>)
    at /home/wesj/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:3940
#61 0x4ad6f68a in GeckoStart (data=0x444fd0, appData=0x814211d4) at /home/wesj/src/mozilla-central/toolkit/xre/nsAndroidStartup.cpp:73
#62 0x814111e6 in Java_org_mozilla_gecko_GeckoAppShell_nativeRun (jenv=0x3442d0, jc=<optimized out>, jargs=0x4063ec70)
    at /home/wesj/src/mozilla-central/mozglue/android/APKOpen.cpp:981
#63 0xaca11e78 in ?? ()
#64 0xaca11e78 in ?? ()

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to Wesley Johnston (:wesj) from comment #16)
> Locally I've been breaking stuff to look into this just a bit move. There
> are still calls to PresShell::FrameNeedsReflow reflows that occur in our
> current code because of calls to 1.) elementFromPoint and 2.)
> getClientRects().
> 
> Removing those (and breaking text selection in the process), we still reflow
> though! and on these particular pages that apparently means multisecond
> pauses. I grabbed a backtrace from this reflows because the profiler didn't
> have much info. It looks like just sending the mouse event causes this to
> run?

That makes some sense to me. Presumably we need to reflow inorder to do hit testing properly. Perhaps a better way to approach this is try to make it so that the mutation we do doesn't require reflow.

Comment 18

[Adrian Tamas (:AdrianT)]

It's almost impossible to move the text selection handles at nytimes on Nightly 16.0a1 2012-06-21 which is the build text selection was introduced in.

For the behavior described in comment 10 the regression range is:
Good Build: May 17 2012
Bad Build: May 18 2012

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c61e7c3a232a&tochange=0c7e2911be75

Comment 19

[Martijn Wargers (dead)]

Adrian, the pushlog link seems to be from april?

Comment 20

[Wesley Johnston (:wesj)]

Please file a separate bug to track that over painting problem.

Comment 21

[Adrian Tamas (:AdrianT)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #19)
> Adrian, the pushlog link seems to be from april?

Sorry about my mistake. The pushlog is correct the build dates were wrong. Correct build dates
Good build: 2012-04-17
Bad build: 2012-04-18

Comment 22

[Martijn Wargers (dead)]

(In reply to Wesley Johnston (:wesj) from comment #20)
> Please file a separate bug to track that over painting problem.

Ok, filed bug 775146 for it.

Comment 23

[Alex Keybl [:akeybl]]

Bug 773100 has landed and was suspected of helping with this issue. Can we get some QA to verify?

Comment 24

[Kevin Brosnan [Ex-Mozilla]]

Yes I have used Margret's test builds and they are much faster/responsive.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

I'm untracking this bug since it's not being used to land any actual fixes, we'll be tracking other bugs for this issue like bug 773100 and bug 772140.