Last Comment Bug 758635 - GeckoLayerClient.viewportSizeChanged gets run twice on surface size changes
: GeckoLayerClient.viewportSizeChanged gets run twice on surface size changes
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 15
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on: 757944 760226
Blocks: 758633
  Show dependency treegraph
 
Reported: 2012-05-25 08:02 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-06-18 12:17 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
.N+


Attachments
(1/2) Remove unnecessary call (2.32 KB, patch)
2012-05-25 08:08 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
(2/2) Remove unnecessary interface implementation (3.71 KB, patch)
2012-05-25 08:09 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
(1/2) Remove unnecessary interface implementation (4.55 KB, patch)
2012-06-01 17:50 PDT, Kartikaya Gupta (email:kats@mozilla.com)
jmuizelaar: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
(2/2) Remove unnecessary call to resizeView (3.32 KB, patch)
2012-06-01 17:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
jmuizelaar: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2012-05-25 08:02:27 PDT
First it gets run here:

05-25 15:00:45.570 E/staktrace(12297): java.lang.Exception
05-25 15:00:45.570 E/staktrace(12297): 	at org.mozilla.gecko.GeckoAppShell.viewSizeChanged(GeckoAppShell.java:1961)
05-25 15:00:45.570 E/staktrace(12297): 	at org.mozilla.gecko.gfx.GeckoLayerClient.viewportSizeChanged(GeckoLayerClient.java:152)
05-25 15:00:45.570 E/staktrace(12297): 	at org.mozilla.gecko.gfx.LayerController.setViewportSize(LayerController.java:154)
05-25 15:00:45.570 E/staktrace(12297): 	at org.mozilla.gecko.gfx.GeckoLayerClient.surfaceChanged(GeckoLayerClient.java:462)
05-25 15:00:45.570 E/staktrace(12297): 	at org.mozilla.gecko.gfx.LayerView.surfaceChanged(LayerView.java:209)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.SurfaceView.updateWindow(SurfaceView.java:544)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.SurfaceView.setFrame(SurfaceView.java:278)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.View.layout(View.java:11272)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.RelativeLayout.onLayout(RelativeLayout.java:925)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.View.layout(View.java:11278)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.ViewGroup.layout(ViewGroup.java:4224)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1628)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1486)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.LinearLayout.onLayout(LinearLayout.java:1399)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.View.layout(View.java:11278)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.ViewGroup.layout(ViewGroup.java:4224)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.FrameLayout.onLayout(FrameLayout.java:431)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.View.layout(View.java:11278)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.ViewGroup.layout(ViewGroup.java:4224)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1628)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1486)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.LinearLayout.onLayout(LinearLayout.java:1399)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.View.layout(View.java:11278)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.ViewGroup.layout(ViewGroup.java:4224)
05-25 15:00:45.570 E/staktrace(12297): 	at android.widget.FrameLayout.onLayout(FrameLayout.java:431)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.View.layout(View.java:11278)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.ViewGroup.layout(ViewGroup.java:4224)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:1489)
05-25 15:00:45.570 E/staktrace(12297): 	at android.view.ViewRootImpl.handleMessage(ViewRootImpl.java:2442)
05-25 15:00:45.570 E/staktrace(12297): 	at android.os.Handler.dispatchMessage(Handler.java:99)
05-25 15:00:45.570 E/staktrace(12297): 	at android.os.Looper.loop(Looper.java:137)
05-25 15:00:45.570 E/staktrace(12297): 	at android.app.ActivityThread.main(ActivityThread.java:4424)
05-25 15:00:45.570 E/staktrace(12297): 	at java.lang.reflect.Method.invokeNative(Native Method)
05-25 15:00:45.570 E/staktrace(12297): 	at java.lang.reflect.Method.invoke(Method.java:511)
05-25 15:00:45.570 E/staktrace(12297): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:784)
05-25 15:00:45.570 E/staktrace(12297): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
05-25 15:00:45.570 E/staktrace(12297): 	at dalvik.system.NativeStart.main(Native Method)


and then it gets run here:

05-25 15:00:45.835 E/staktrace(12297): 	at org.mozilla.gecko.GeckoAppShell.viewSizeChanged(GeckoAppShell.java:1961)
05-25 15:00:45.835 E/staktrace(12297): 	at org.mozilla.gecko.gfx.GeckoLayerClient.viewportSizeChanged(GeckoLayerClient.java:152)
05-25 15:00:45.835 E/staktrace(12297): 	at org.mozilla.gecko.gfx.LayerController.setViewportSize(LayerController.java:154)
05-25 15:00:45.835 E/staktrace(12297): 	at org.mozilla.gecko.gfx.LayerView.setViewportSize(LayerView.java:95)
05-25 15:00:45.835 E/staktrace(12297): 	at org.mozilla.gecko.gfx.LayerRenderer$1.run(LayerRenderer.java:344)
05-25 15:00:45.835 E/staktrace(12297): 	at android.os.Handler.handleCallback(Handler.java:605)
05-25 15:00:45.835 E/staktrace(12297): 	at android.os.Handler.dispatchMessage(Handler.java:92)
05-25 15:00:45.835 E/staktrace(12297): 	at android.os.Looper.loop(Looper.java:137)
05-25 15:00:45.835 E/staktrace(12297): 	at android.app.ActivityThread.main(ActivityThread.java:4424)
05-25 15:00:45.835 E/staktrace(12297): 	at java.lang.reflect.Method.invokeNative(Native Method)
05-25 15:00:45.835 E/staktrace(12297): 	at java.lang.reflect.Method.invoke(Method.java:511)
05-25 15:00:45.835 E/staktrace(12297): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:784)
05-25 15:00:45.835 E/staktrace(12297): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
05-25 15:00:45.835 E/staktrace(12297): 	at dalvik.system.NativeStart.main(Native Method)

This results in two ScrollTo:FocusedInput messages getting sent to browser.js. The second one of these is completely redundant.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-25 08:08:54 PDT
Created attachment 627234 [details] [diff] [review]
(1/2) Remove unnecessary call
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-25 08:09:15 PDT
Created attachment 627235 [details] [diff] [review]
(2/2) Remove unnecessary interface implementation
Comment 3 Chris Lord [:cwiiis] 2012-05-26 01:43:24 PDT
Comment on attachment 627234 [details] [diff] [review]
(1/2) Remove unnecessary call

Review of attachment 627234 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine.
Comment 4 Chris Lord [:cwiiis] 2012-05-26 01:54:50 PDT
Comment on attachment 627235 [details] [diff] [review]
(2/2) Remove unnecessary interface implementation

Review of attachment 627235 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine with comments addressed.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +198,5 @@
>              super.finalize();
>          }
>      }
>  
> +    void onSurfaceCreated(GL10 gl, EGLConfig config) {

This is still public isn't it...?

@@ -273,5 @@
>  
> -    /**
> -     * Called whenever a new frame is about to be drawn.
> -     */
> -    public void onDrawFrame(GL10 gl) {

Glad you've removed this, was very misleading...

@@ +309,5 @@
>          return new RenderContext(viewport, pageSize, new IntSize(mSurfaceWidth, mSurfaceHeight), zoomFactor, mPositionHandle, mTextureHandle,
>                                   mCoordBuffer);
>      }
>  
> +    void onSurfaceChanged(GL10 gl, final int width, final int height) {

Same here.
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-26 08:02:06 PDT
(In reply to Chris Lord [:cwiiis] from comment #4)
> Comment on attachment 627235 [details] [diff] [review]
> > +    void onSurfaceCreated(GL10 gl, EGLConfig config) {
> 
> This is still public isn't it...?
> > +    void onSurfaceChanged(GL10 gl, final int width, final int height) {
> 
> Same here.

It's package-scope now. Since it's no longer needed to implement an interface method, it doesn't need to be public. It's called from GLController which is in the same package so package-scope is sufficient visibility for this.
Comment 7 Phil Ringnalda (:philor) 2012-05-26 13:23:28 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a4961783ef - reftests started failing around edges and borders, rather oddly. My first guess was what it always is with Fennec, clobber, since there was one push above you which got a clobber and got green on all three hunks of reftest, but in fact it wasn't even consistent for a particular build, so apparently it's a problem of a particular run of the build, whether it starts in a way that will produce the same 9 R1 or 26 R2 or 57 R3 failures, or in a way that will be green.
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-30 10:05:46 PDT
Just an update: I've been trying to figure out why the reftests are failing (they do seem to fail quasi-reliably with these patches). I've been focusing on the R1 tests just to drill down and isolate the behaviour. The first of the two patches on this bug is sufficient to trigger the failure [1]. At first I thought it was just a random timing issue so I replaced the code I removed with a small sleep() but that didn't fix the problem.

It seems related to the issue :ajuma and :jmaher were investigating in that there is one test that fails (test-displayport-bg) and seems to cascade and cause other test failures. I've started making changes to that test to get more data about the failure. I currently have four try builds going to get different pieces of data:
- Disabling the test-displayport-bg test [2]. So far it looks like this fixes all the failures, but I'm running more instances of the R1 test to verify
- Changing the size of the displayport set in the test [3]. This seems to fix the test-displayport-bg test, but the remaining failures still persist. Again, running more R1 instances to verify.
- Not setting a CSS viewport in the test [4]. No data yet
- Not setting the async scroll flag in the test [5]. No data yet

[1] https://tbpl.mozilla.org/?tree=Try&rev=4b185f8a322d
[2] https://tbpl.mozilla.org/?tree=Try&rev=0ae75730b4fa
[3] https://tbpl.mozilla.org/?tree=Try&rev=e86326ffe31b
[4] https://tbpl.mozilla.org/?tree=Try&rev=4a5f432eb527
[5] https://tbpl.mozilla.org/?tree=Try&rev=02f5b1a957bb
Comment 9 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-31 08:18:53 PDT
Further update:
- Disabling the test-displayport-bg test does not in fact fix the reftests entirely; although it does make them hit orange less frequently. Perhaps the other displayport tests are impacting future tests. Or maybe the other failures are just random. For now I'll go with the former assumption since otherwise I'm likely to just give up.
- Not setting the CSS viewport doesn't help
- Not setting the async scroll flag doesn't help
- I'm trying a build at https://tbpl.mozilla.org/?tree=Try&rev=fcf056af6d35 which removes the displayport params to see if that helps.

My current hypothesis goes something like this: having a test that sets the displayport screws things up. Without the patches in this bug, the Java code sends two sets of resize events on start, and the second one might be clobbering the displayport set by the test. Removing that second resize event, then, allows the dispalyport set by the test to persist and causes these failures.
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-31 12:25:31 PDT
So it turns out the reftests were actually failing because of the code change; the call to viewportSizeChanged that happens during startup actually did stuff, and my change took it out. Discussed with :jrmuizel since he and :BenWa are working on the same code and we decided on a set of changes that we should be able to make here that cleans things up and also solves this duplicate viewportSizeChanged call.
Comment 11 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-01 17:50:28 PDT
Created attachment 629417 [details] [diff] [review]
(1/2) Remove unnecessary interface implementation
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-01 17:51:21 PDT
Created attachment 629418 [details] [diff] [review]
(2/2) Remove unnecessary call to resizeView

Try build with these patches (plus the ones BenWa previously pushed) is at https://tbpl.mozilla.org/?tree=Try&rev=edad3477d6f6 and looks pretty promising.
Comment 13 Jeff Muizelaar [:jrmuizel] 2012-06-02 09:17:02 PDT
Comment on attachment 629417 [details] [diff] [review]
(1/2) Remove unnecessary interface implementation

Lovely
Comment 14 Jeff Muizelaar [:jrmuizel] 2012-06-02 09:23:47 PDT
Comment on attachment 629418 [details] [diff] [review]
(2/2) Remove unnecessary call to resizeView

And also very nice.
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-03 14:51:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c7d83e5cb2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1d53c1f729

Thanks for the prompt reviews! :)
Comment 17 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-06 07:03:09 PDT
Comment on attachment 629417 [details] [diff] [review]
(1/2) Remove unnecessary interface implementation

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: showing/hiding the keyboard does unnecessary work and is therefore unnecessarily slow. it might also be the cause of glitches
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): mobile-only. should be fairly low risk
String or UUID changes made by this patch: none

I'm not really sure if this is worth uplifting but putting it out there anyway because it might help prevent things like bug 758190 which at one point was a blocker. The two dependencies of this patch have both been uplifted.
Comment 18 Alex Keybl [:akeybl] 2012-06-18 11:35:33 PDT
Comment on attachment 629417 [details] [diff] [review]
(1/2) Remove unnecessary interface implementation

[Triage Comment]
Approving for 14.0.1 since this is low risk and will make keyboard bring-up less glitchy and more "performant".
Comment 19 Alex Keybl [:akeybl] 2012-06-18 11:37:35 PDT
Please only land on mozilla-beta tip.
Comment 20 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-18 12:00:36 PDT
These patches don't apply cleanly now; I would like to get the patch from bug 753458 in first to minimize rebase error. I requested beta approval for that patch.
Comment 21 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-18 12:16:49 PDT
Landed along with above-mentioned patch from bug 753458 onto the default branch of mozilla-beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/95f8c99b4db0
https://hg.mozilla.org/releases/mozilla-beta/rev/61757c94dd74

Note You need to log in before you can comment on or make changes to this bug.