844275 - Fix ALL the compositor startup bugs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

These are the things that should work without ending up a "permanent" bad state (i.e. one that requires user interaction to fix):

- Start fennec and load a page from thumbnails
- Start fennec, open awesomebar, load a page before gecko is up
- Start fennec, open awesomebar, wait for gecko to come up, then load a page
- After loading a page in portrait, rotate to landscape
- After loading a page in landscape, rotate to portrait
- After loading a page in portrait, go to homescreen and back
- After loading a page in landscape, go to homescreen and back
- After loading a page in portrait, go to homescreen, rotate the device to landscape, and back into Fennec
- After loading a page in landscape, go to homescreen, rotate the device to portrait, and back into Fennec
- Showing/hiding the keyboard while on a page
- Going to homescreen and back while the keyboard is showing
- Rotation while the keyboard is showing
- Anything else you can think of
- All the above scenarios, but with the "Don't keep activities" box checked

Also, TBPL tests.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Teaser: https://tbpl.mozilla.org/?tree=Try&rev=51063e442119

Comment 2

[Kartikaya Gupta (email:[e-mail])]

So... the try run above is orange for all the tegra tests, but the log appears to have absolutely zero useful information. jmaher/gbrown, any ideas? I don't even know how to start debugging this. I suspect it'll be simple to fix once I figure out what the problem is but the logging in the test framework is inadequate to let me diagnose the problem. The try build itself works fine on my device (and the pandas) so it's something tegra-specific.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

installed this on my local tegra and found this in adb logcat:

01-10 05:23:02.186 I/SUTAgentAndroid( 1592): 192.168.1.73 : exec org.mozilla.fennec
01-10 05:23:02.277 I/ActivityManager( 1031): Starting activity: Intent { act=android.intent.action.MAIN flg=0x10000000 pkg=org.mozilla.fennec cmp=org.mozilla.fennec/.App }
01-10 05:23:02.306 I/UsageStats( 1031): Deleting usage file : usage-19700101
01-10 05:23:02.316 I/ActivityManager( 1031): Start proc org.mozilla.fennec for activity org.mozilla.fennec/.App: pid=20379 uid=10033 gids={3003, 1015, 1006}
01-10 05:23:02.456 I/ActivityThread(20379): Publishing provider org.mozilla.fennec.db.formhistory: org.mozilla.fennec.db.FormHistoryProvider
01-10 05:23:02.456 I/ActivityThread(20379): Publishing provider org.mozilla.fennec.db.browser: org.mozilla.fennec.db.BrowserProvider
01-10 05:23:02.466 I/ActivityThread(20379): Publishing provider org.mozilla.fennec.db.tabs: org.mozilla.fennec.db.TabsProvider
01-10 05:23:02.546 I/GeckoProfile(20379): New installation or update, checking for old profiles.
01-10 05:23:02.586 D/GeckoFavicons(20379): Creating Favicons instance
01-10 05:23:02.586 D/GeckoLoader(20379): Gecko environment env0: null
01-10 05:23:02.586 I/GeckoApp(20379): froyo startup fix: com.android.internal.view.InputBindResult$1@4437b2a0
01-10 05:23:02.676 D/GeckoViewsFactory(20379): Warning: unknown custom view: org.mozilla.gecko.TabsPanel$TabsPanelToolbar
01-10 05:23:02.686 D/GeckoViewsFactory(20379): Warning: unknown custom view: org.mozilla.gecko.TabsPanel$TabsListContainer
01-10 05:23:02.776 D/libEGL  (20379): loaded /system/lib/egl/libGLES_android.so
01-10 05:23:02.776 D/libEGL  (20379): loaded /system/lib/egl/libEGL_tegra.so
01-10 05:23:02.796 D/libEGL  (20379): loaded /system/lib/egl/libGLESv1_CM_tegra.so
01-10 05:23:02.796 D/libEGL  (20379): loaded /system/lib/egl/libGLESv2_tegra.so
01-10 05:23:02.806 D/AndroidRuntime(20379): Shutting down VM
01-10 05:23:02.816 E/GeckoAppShell(20379): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
01-10 05:23:02.816 E/GeckoAppShell(20379): org.mozilla.gecko.gfx.GLController$GLControllerException: No available EGL configurations Error 12289
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at org.mozilla.gecko.gfx.GLController.chooseConfig(GLController.java:177)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at org.mozilla.gecko.gfx.GLController.initEGL(GLController.java:170)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at org.mozilla.gecko.gfx.GLController.surfaceChanged(GLController.java:101)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at org.mozilla.gecko.gfx.LayerView.surfaceChanged(LayerView.java:382)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at org.mozilla.gecko.gfx.LayerView.access$500(LayerView.java:44)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at org.mozilla.gecko.gfx.LayerView$LayerSurfaceView.onLayout(LayerView.java:446)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.View.layout(View.java:7035)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.FrameLayout.onLayout(FrameLayout.java:333)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.View.layout(View.java:7035)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.RelativeLayout.onLayout(RelativeLayout.java:909)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.View.layout(View.java:7035)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1249)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1125)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.LinearLayout.onLayout(LinearLayout.java:1042)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.View.layout(View.java:7035)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.RelativeLayout.onLayout(RelativeLayout.java:909)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.View.layout(View.java:7035)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.FrameLayout.onLayout(FrameLayout.java:333)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.View.layout(View.java:7035)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.widget.FrameLayout.onLayout(FrameLayout.java:333)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.View.layout(View.java:7035)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.ViewRoot.performTraversals(ViewRoot.java:1045)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.view.ViewRoot.handleMessage(ViewRoot.java:1727)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.os.Handler.dispatchMessage(Handler.java:99)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.os.Looper.loop(Looper.java:123)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at android.app.ActivityThread.main(ActivityThread.java:4627)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at java.lang.reflect.Method.invokeNative(Native Method)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at java.lang.reflect.Method.invoke(Method.java:521)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:868)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:626)
01-10 05:23:02.816 E/GeckoAppShell(20379): 	at dalvik.system.NativeStart.main(Native Method)
01-10 05:23:03.086 D/GeckoProfile(20379): Created new profile dir at /data/data/org.mozilla.fennec/files/mozilla/9gloxt04.default

The process is still running, but that is all that I see in the log files.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Awesome, thanks for the quick response! I don't know if it's possible to get that logcat output in the tbpl output but that would be really handy.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I've been working on this, my current WIP queue is at https://github.com/staktrace/mozilla-central/commits/compositor. However I'm running a problem where some mochitests are consistently failing. There are try runs I've pushed here:

https://tbpl.mozilla.org/?tree=Try&rev=829b88f0a037
https://tbpl.mozilla.org/?tree=Try&rev=1cb18a69910b
https://tbpl.mozilla.org/?tree=Try&rev=df2e0ff30bc1

It looks like almost all of the remaining test failures are related in some way to the MozAfterPaint event, and could potentially be caused by that not firing properly. In https://tbpl.mozilla.org/php/getParsedLog.php?id=20024295&tree=Try&full=1#error0 I see this exception:

org.mozilla.gecko.gfx.GLController$GLControllerException: No EGL configuration for that specification Error 12293

which is just bizarre because it means that the config returned by eglChooseConfig cannot actually be chosen. That this is happening only on the Tegras and only intermittently is even more puzzling. My current thinking is that somehow the code running in the GfxInfoThread is interfering with the init code in GLController, causing a failure. If we fail to init the compositor it remains in the paused state, we never paint anything, and MozAfterPaint will never fire. So in theory this could explain everything but I need to come up with a way to verify and/or fix it.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

So my hypothesis about GfxInfoThread turned out to be correct, sort of. I misunderstood what GfxInfoThread was doing and didn't realize it always had to run ahead of creating the compositor. With that fixed everything is starting to look good. Who wants to review the flood of patches? :D

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Start GLC's ImmutableViewportMetrics with the right viewport size

This code is left over from back in the day when we created LayerView manually in code and so didn't know its size until much later. Now we know it on startup and can use it there. Future patches in the series require this.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Move compositor-related things from GLC to the other GLC

Move some stuff from GeckoLayerClient to GLController. It's intended to be a no-functional-change kind of patch, but I'm not entirely sure that's true without the other patches in the series. I wouldn't land it by itself.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Make GLController a singleton

This resolves problems of the sort mentioned in bug 842946 comment 7. We only ever create one C++ compositor over Fennec's lifetime, and this GLController instance matches that on the Java side, so the mCompositorCreated field and other state variables are always accurate.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Drive layer manager creation from GLController

This patch is the meat of this patch queue. The fundamental change here is that instead of creating the layer manager and compositor on the first call to GetLayerManager (which happens randomly from our point of view), we create it synchronously when both of the following conditions are satisfied:
1) Gecko is ready
2) We have a GL surface available in Java

Because creating the compositor requires a GL surface, we used to block on compositor creation until a surface was available. Now we just return a null LayerManager until we're good and ready to create the compositor, and then we do it synchronously from the Java UI thread so that the two conditions above cannot be violated during the creation.

This also eliminates the "waitForValidSurface" codepath that used to exist, because we now are guaranteed that the surface will be valid when we go to create the compositor.

Also we start the nsWindow::sCompositorPaused in a true state, so that Gecko draws are paused initially (via nsWindow::NeedsPaint).

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Fold AndroidLayerView* into AndroidBridge

This is just some cleanup. There's very little actual code in AndroidLayerViewWrapper.* so I collapsed it into AndroidBridge.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 6 - Update test_acceleration to deal with a null layer manager

This was the only real fallout from making GetLayerManager return null early in Gecko startup (part 4 of this patch series).

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 7 - Move GfxInfoThread so it still runs before compositor startup

After all the previous changes, I found that the EGL init stuff in GLContoller was happening in parallel with the GfxInfoThread data-gathering and one or both would fail due to interference. This moves the GfxInfoThread to start up right away and blocks the egl init in GLController until it is done.

Note that this patch requires bug 845877 to apply properly.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

And that's it! Try build at https://tbpl.mozilla.org/?tree=Try&rev=e11b8f2109a5 if you want to admire the green-ness or take it for a spin.

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 719303 [details] [diff] [review]
Part 1 - Start GLC's ImmutableViewportMetrics with the right viewport size

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

Looks reasonable to me.

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 719303 [details] [diff] [review]
Part 1 - Start GLC's ImmutableViewportMetrics with the right viewport size

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

Looks reasonable to me.

Edit: One further note, would it be sensible to modify the ImmutableViewportMetrics functions that return new metrics to check if they're different first and return the same object if they're not?

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 719304 [details] [diff] [review]
Part 2 - Move compositor-related things from GLC to the other GLC

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

Much nicer.

Comment 18

[Chris Lord [:cwiiis]]

Comment on attachment 719308 [details] [diff] [review]
Part 3 - Make GLController a singleton

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

Again, nicer. Might be good to add a comment somewhere in GLC explaining why it's a singleton, and what it does? Can be addressed in follow-up though.

Comment 19

[Chris Lord [:cwiiis]]

Comment on attachment 719310 [details] [diff] [review]
Part 4 - Drive layer manager creation from GLController

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

Good, but a few things to address before checking in.

::: mobile/android/base/gfx/GLController.java
@@ +97,5 @@
> +        // So we're going to create the window surface and hold on to it until
> +        // the compositor comes asking for it. However, we can't call eglCreateWindowSurface
> +        // right away because the UI thread isn't done setting up, so we need
> +        // to stuff in a runnable posted back to the UI thread that runs once
> +        // the setup is done.

Might also be good to add a reference to what the UI thread is doing in terms of Class/method.

@@ +104,5 @@
> +            public void run() {
> +                try {
> +                    // Re-check mSurfaceValid in case the surface was destroyed between setting it
> +                    // above and here. If mSurfaceValid is false, or if the eglCreateWindowSurface
> +                    // call fails, mEGLSurface will be null.

This comment is a little confusing as it says what happens when mSurfaceValid is false, but the following block is doing stuff only if it's true. I understand it, but I think rewording here might be good. Perhaps,

// Re-check mSurfaceValid in case the surface was destroyed between setting it
// above and here. If it is still valid, try to create mEGLSurface. mEGLSurface
// will be null if eglCreateWindowSurface fails, or if mSurfaceValid had become
// invalid.

Even this isn't quite true, as it might be EGL10.EGL_NO_SURFACE, as seen below... Feel free to reword if this bothers you :)

@@ +142,5 @@
> +        // Only try to create the compositor if we have a valid surface and gecko is up. When these
> +        // two conditions are satisfied, we can be relatively sure that the compositor creation will
> +        // happen without needing to block anyhwere. Do it with a sync gecko event so that the
> +        // android doesn't have a chance to destroy our surface in between.
> +        if (mEGLSurface != null && GeckoThread.checkLaunchState(GeckoThread.LaunchState.GeckoRunning)) {

Could mEGLSurface possibly be EGL10.EGL_NO_SURFACE here?

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +125,5 @@
>  
>          sendResizeEventIfNecessary(true);
>  
>          DisplayPortCalculator.initPrefs();
> +        mView.post(new Runnable() {

Let's have a comment here as to what's going on and why it's posted to the UI thread.

::: widget/android/nsWindow.cpp
@@ +692,5 @@
> +    // shared across all windows
> +    if (UseOffMainThreadCompositing()) {
> +        return sLayerManager;
> +    }
> +    return nullptr;

previously GetLayerManager would have tried to create the layer manager if there wasn't one - Will this interfere with xul-fennec? (do we care anymore? (if we don't, is there any point in having all of this legacy code around?))

Comment 20

[Chris Lord [:cwiiis]]

Comment on attachment 719311 [details] [diff] [review]
Part 5 - Fold AndroidLayerView* into AndroidBridge

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

Looks good, nice to see the code size reducing :)

::: widget/android/AndroidBridge.cpp
@@ +1143,5 @@
>      jobject glController = env->CallStaticObjectMethod(jLayerView, registerCompositor);
>      if (jniFrame.CheckForException())
>          return;
>  
> +    mGLControllerObj = env->NewGlobalRef(glController);

Does the jobject destructor release the reference? I just worry about leaking a ref if this gets called twice for whatever reason. You've probably thought of this already.

@@ +1149,5 @@
>  
>  EGLSurface
>  AndroidBridge::ProvideEGLSurface()
>  {
> +    if (!jEGLSurfacePointerField) {

Should this also check that mGLControllerObj is non-null?

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 719316 [details] [diff] [review]
Part 7 - Move GfxInfoThread so it still runs before compositor startup

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

LGTM, but a second pair of eyes would be good.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #16)
> Edit: One further note, would it be sensible to modify the
> ImmutableViewportMetrics functions that return new metrics to check if
> they're different first and return the same object if they're not?

Good idea, I've filed bug 846355 to track that.

(In reply to Chris Lord [:cwiiis] from comment #18)
> Again, nicer. Might be good to add a comment somewhere in GLC explaining why
> it's a singleton, and what it does? Can be addressed in follow-up though.

I added a class-level comment in GLController explaining what I said in comment 9 in a bit more detail.

(In reply to Chris Lord [:cwiiis] from comment #19)
> Might also be good to add a reference to what the UI thread is doing in
> terms of Class/method.

You mean in the Android code? I didn't look into that too much; however I fleshed out the comment a bit to explain things some more.

> @@ +104,5 @@
> This comment is a little confusing as it says what happens when
> mSurfaceValid is false, but the following block is doing stuff only if it's
> true. I understand it, but I think rewording here might be good. Perhaps,
> 
> // Re-check mSurfaceValid in case the surface was destroyed between setting
> it
> // above and here. If it is still valid, try to create mEGLSurface.
> mEGLSurface
> // will be null if eglCreateWindowSurface fails, or if mSurfaceValid had
> become
> // invalid.
> 
> Even this isn't quite true, as it might be EGL10.EGL_NO_SURFACE, as seen
> below... Feel free to reword if this bothers you :)

Good call, thanks. I've reworded it to be more clear.

> @@ +142,5 @@
> 
> Could mEGLSurface possibly be EGL10.EGL_NO_SURFACE here?

Good catch! I changed the code slightly to set mEGLSurface to null if the result from eglCreateWindowSurface was EGL_NO_SURFACE. That way I can just leave the comparison to null here.

> 
> Let's have a comment here as to what's going on and why it's posted to the
> UI thread.

Done.

> ::: widget/android/nsWindow.cpp
> previously GetLayerManager would have tried to create the layer manager if
> there wasn't one - Will this interfere with xul-fennec? (do we care anymore?
> (if we don't, is there any point in having all of this legacy code around?))

We don't care about XUL-fennec anymore. I think we can probably kill some of this code, but I wasn't sure exactly which pieces I could kill and which are actually still useful fallback paths, so I left it as-is.


(In reply to Chris Lord [:cwiiis] from comment #20)
> >  
> > +    mGLControllerObj = env->NewGlobalRef(glController);
> 
> Does the jobject destructor release the reference? I just worry about
> leaking a ref if this gets called twice for whatever reason. You've probably
> thought of this already.

Good point. It shouldn't get called twice, but I'll add an assert to catch that scenario. There's no sane place to release the ref though; the jobject destructor won't clean that up. It's effectively a shutdown leak that we don't care about.

> > +    if (!jEGLSurfacePointerField) {
> 
> Should this also check that mGLControllerObj is non-null?

Also that should never happen; I'll add an assert to catch it if it does.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> (In reply to Chris Lord [:cwiiis] from comment #20)
> > >  
> > > +    mGLControllerObj = env->NewGlobalRef(glController);
> > 
> > Does the jobject destructor release the reference? I just worry about
> > leaking a ref if this gets called twice for whatever reason. You've probably
> > thought of this already.
> 
> Good point. It shouldn't get called twice, but I'll add an assert to catch
> that scenario. There's no sane place to release the ref though; the jobject
> destructor won't clean that up. It's effectively a shutdown leak that we
> don't care about.
> 

Looking at the code a bit more I wasn't 100% sure that it won't get called more than once. I've made subsequent calls just return early for more robust handling.

Landed on inbound along with the patch from bug 845877.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f95e11792cfd
https://hg.mozilla.org/integration/mozilla-inbound/rev/22e3d1308e98
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3b5a25bcae
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5fe1593b6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bcca41d551
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d76808a48e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/eadd24d27a61

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f95e11792cfd
https://hg.mozilla.org/mozilla-central/rev/22e3d1308e98
https://hg.mozilla.org/mozilla-central/rev/fc3b5a25bcae
https://hg.mozilla.org/mozilla-central/rev/cd5fe1593b6f
https://hg.mozilla.org/mozilla-central/rev/c1bcca41d551
https://hg.mozilla.org/mozilla-central/rev/0d76808a48e9
https://hg.mozilla.org/mozilla-central/rev/eadd24d27a61

Comment 25

[Richard Newman [:rnewman]]

What's the plan to get these patches uplifted to Aurora?

We're tracking Bug 849658 for fx21, and it's probably a dupe of this octo-bug, in the form of Bug 758190. It's a pretty hideous user experience, and we don't know how widespread it is.

(It certainly hits me on the most popular current Android device.)

Comment 26

[Kartikaya Gupta (email:[e-mail])]

I would like to uplift to Aurora, yes, but I just want to make sure there's (a) no bad fallout and (b) it actually fixes all the things it claims to fix. In terms of fallout, bug 846774 just landed on central which would need to be uplifted together with this. bug 847002 also appears to have been introduced by this bug and is the #1 topcrasher on nightly right now, but I don't know what conditions cause it to trigger.

Comment 27

[Richard Newman [:rnewman]]

OK, flagging for tracking.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 719303 [details] [diff] [review]
Part 1 - Start GLC's ImmutableViewportMetrics with the right viewport size

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: many graphical glitches (see bug dependency list)
Testing completed (on m-c, etc.): on m-c, aurora try build at https://tbpl.mozilla.org/?tree=Try&rev=26b79db37644
Risk to taking this patch (and alternatives if risky): medium risk, fennec only. potentially increases crash rate of bug 847002 (#1 topcrasher on nightly). no other known regressions.
String or UUID changes made by this patch: none

Comment 29

[bhavana bajaj [:bajaj]]

> Risk to taking this patch (and alternatives if risky): medium risk, fennec
> only. potentially increases crash rate of bug 847002 (#1 topcrasher on
> nightly). no other known regressions.

Looks like we have got STR https://bugzilla.mozilla.org/show_bug.cgi?id=847002#c7 in this bug recently.I am holding off the approval for a couple of days to see if the investigation evolves into a fix to avoid introducing a known top-crasher for aurora users.

Comment 30

[bhavana bajaj [:bajaj]]

Comment on attachment 719303 [details] [diff] [review]
Part 1 - Start GLC's ImmutableViewportMetrics with the right viewport size

Bug 847002(A fallout from this bug) is now resolved on trunk with no new crashes after the builddate 0315.

This patch is ready to be uplifted on aurora as it helps fix known startup issues which have been there for a while .The risk is manageable considering the baked time it has had on central and the fallouts have been addressed .

Comment 31

[bhavana bajaj [:bajaj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> Comment on attachment 719303 [details] [diff] [review]
> Part 1 - Start GLC's ImmutableViewportMetrics with the right viewport size
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: many graphical glitches (see bug dependency list)
> Testing completed (on m-c, etc.): on m-c, aurora try build at
> https://tbpl.mozilla.org/?tree=Try&rev=26b79db37644

 Kats, can you please add qawanted on this bug if there is a need for any exploratory testing around the testcases provided in the description. I am not sure how well those cases have been tested, hence depending on that we could request QA help here.

I will already request QA verification on a few blocker bugs that this Bug & its friends may have resolved .

Comment 32

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b83927fb91f
https://hg.mozilla.org/releases/mozilla-aurora/rev/79ac9502c85d
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca5b540c879e
https://hg.mozilla.org/releases/mozilla-aurora/rev/684bbe5d51a0
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd0c3b1d7f21
https://hg.mozilla.org/releases/mozilla-aurora/rev/314d4dd7dc73
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8c1062ccaf0

Comment 33

[Kartikaya Gupta (email:[e-mail])]

I'd like QA to verify that the scenarios described in comment 0 all result in proper behaviour on Fx 21 post-uplift. That should be a good baseline behaviour verification; I'll keep an eye out for any intermittent failures or bugs filed against Fx 21 that might be related to these patches.

Comment 34

[Paul Feher]

Verified this on:
Nightly 22.0a1 (2013-03-19)
Aurora 21.0a2 (2013-03-19)
Samsung Galaxy SII (Android 4.0.3) and Samsung Galaxy Tab (Android 4.0.4)
Following the scenarios from comment0 there were no new issues encountered.