Text selection performance is bad on android

VERIFIED FIXED

Status

()

Firefox for Android
Text Selection
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

(Depends on: 1 bug)

unspecified
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15- fixed, firefox16- verified)

Details

(Whiteboard: [sps])

Attachments

(2 attachments)

(Assignee)

Description

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

Profile coming...
(Assignee)

Updated

5 years ago
Depends on: 767073
Excessive screenshots symptom? Faster with it disabled?
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
Here's the profile:
 http://people.mozilla.com/~bgirard/cleopatra/?report=AMIfv94qINFsDl8v4YsWAhVjKSVfyutKyttETseFkUQ-p-JpIXKJBVkLbJwrkC1nVEtDHFNRXA85GzgWlX6t-dnzNK6bDcOPhyX6Wc4B5g__AtjiIWJwLEC46p2hkzeOKsimebHkWSG0kzvya1NoQOdhVhNIDpcHaA
(Assignee)

Updated

5 years ago
Depends on: 767085

Updated

5 years ago
Blocks: 695173

Comment 4

5 years ago
Jeff, do you know who would be able to help us out with this? Also, is this the same issue causing bug 772140?
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?
Keywords: testcase-wanted

Updated

5 years ago
Duplicate of this bug: 769884

Comment 7

5 years ago
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.
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
Component: General → Text Selection
OS: Mac OS X → Android
Hardware: x86 → ARM

Updated

5 years ago
tracking-firefox15: ? → +
tracking-firefox16: ? → +
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → jmuizelaar
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
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.
A regression range might be useful here.
Keywords: regressionwindow-wanted
(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.
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.
Created attachment 642048 [details] [diff] [review]
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
I should say that makes text selection not work well. It works, but with a few quirks.

Updated

5 years ago
Depends on: 773730

Updated

5 years ago
Depends on: 773813

Updated

5 years ago
Depends on: 773819

Updated

5 years ago
Depends on: 773864
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 ?? ()
(Assignee)

Comment 17

5 years ago
(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.
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
Keywords: regressionwindow-wanted
Adrian, the pushlog link seems to be from april?
Please file a separate bug to track that over painting problem.

Updated

5 years ago
Depends on: 774938
(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

Updated

5 years ago
Depends on: 775146
(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.

Updated

5 years ago
No longer depends on: 775146

Updated

5 years ago
Depends on: 773100
Bug 773100 has landed and was suspected of helping with this issue. Can we get some QA to verify?
Keywords: qawanted
Yes I have used Margret's test builds and they are much faster/responsive.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
status-firefox15: affected → fixed
status-firefox16: affected → fixed
tracking-firefox15: + → -
tracking-firefox16: + → -

Updated

5 years ago
Whiteboard: [sps]
Keywords: qawanted, testcase-wanted

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
You need to log in before you can comment on or make changes to this bug.