852526 - Launching a web apps for the first time displays white screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Paul Feher]

Nightly Fennec 16.0a1 (2013-03-19)
Devices: Samsung Galxy SII (Android 4.0.3), Samsung Galaxy Tab (4.0.4), HTC Desire Z (Android 2.3.3)

Steps to Reproduce:

1. Open about:apps
1. Install a web app(ex Twitter)
3. Open the web app.

Expected:
The web app opens correctly.

Actual:
A white screen appears when opening the web app. After tapping the home screen button and reopening the web app the app is loaded correctly.

Comment 1

[Paul Feher]

Sorry, this is reproducible on Nightly 22.0a1 (2013-03-19)

Comment 2

[Aaron Train [:aaronmt]]

You sure it's not crashing? I havn't been able to launch web-apps for a week now, bug 844895

Can you check the logcat?

Comment 3

[Paul Feher]

Attachment: Log File

Comment 4

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

(In reply to Paul Feher from comment #3)
> Created attachment 726663 [details]
> Log File

I see no crashes in the log. If you rotate the device, does the screen paint the webapp?

Comment 5

[Paul Feher]

Yes, after changing orientation the webapp is painted properly

Comment 6

[Aaron Train [:aaronmt]]

Oh snap, I see something related to this on my Motorola Droid Bionic (Android 2.3.4) Nightly (03/19), in about:config, resetting a preference wont update the screen unless I rotate.

Comment 7

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

Probably related to this:

03-20 16:43:04.035 I/CompositorParent(26499): Unable to renew compositor surface; remaining in paused state
03-20 16:43:04.035 I/CompositorParent(26499): Unable to renew compositor surface; remaining in paused state
03-20 16:43:04.035 D/GeckoLayerClient(26499): Window-size changed to (480,430)
03-20 16:43:04.035 D/PowerManagerService( 1974): onSensorChanged: light value: 1000
03-20 16:43:04.080 W/WifiStateTracker( 1974): getNetworkInfo : NetworkInfo: type: WIFI[], state: CONNECTED/CONNECTED, reason: (unspecified), extra: (none), roaming: false, failover: false, isAvailable: true
03-20 16:43:04.095 E/libEGL  (26499): eglMakeCurrent:629 error 3009 (EGL_BAD_MATCH)

I will try to reproduce and debug if I can.

Comment 8

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

Also, can you let me know exactly what you did to reproduce this? e.g. after installing Twitter, did you exit fennec before opening twitter, or did you just put it in the background? did you do any device rotations during that time? as much detail as possible would be appreciated.

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Log-cat (Motorola Droid Bionic 2.3.4)

I see the same thing on my Motorola Droid Bionic (Android 2.3.4)

In the odd case where I don't crash: new profile, launch Nightly, visit the marketplace, install Twitter and launch Twitter

Comment 10

[Paul Feher]

I've tried several methods first launching the app directly by pressing the  launch button immediately after the app was installed without closing or minimizing the application and second by closing firefox and launch the app from the home screen. The result  was the same.

Comment 11

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

I can reproduce the blank screen that shows up after the splash screen goes away. The blank screen is replaced by the twitter sign in page on rotation or home/re-entry. However I don't see the same logcat output so maybe it's a different problem.

Comment 12

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> I can reproduce the blank screen that shows up after the splash screen goes
> away. The blank screen is replaced by the twitter sign in page on rotation
> or home/re-entry. However I don't see the same logcat output so maybe it's a
> different problem.

Sounds exactly like what I see with my installed webapps.

Comment 13

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

This is a regression from bug 850690; the compositor thinks the EGL surface size is 0x0. I guess the codepath that was removed was needed after all. :(

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> This is a regression from bug 850690; the compositor thinks the EGL surface
> size is 0x0. I guess the codepath that was removed was needed after all. :(

Well, at least we know, that makes fixing it a bit easier... Perhaps I can reinstate the old fix, with a fix on top to stop it from squashing subsequent resizes.

Comment 16

[Chris Lord [:cwiiis]]

Reading the code, could the fix be as simple as just setting the surface size on compositor creation? From a cursory look, it seems that doesn't happen...

Comment 17

[Chris Lord [:cwiiis]]

So I think this happens because we don't implement GetBounds in nsWindow and that's what's used to initialise the compositor size when it gets created. If we return gAndroidBounds in GetBounds, this ought to stop a 0x0 surface size when the window size is set before the compositor gets created. Writing/testing a patch now.

Comment 18

[Chris Lord [:cwiiis]]

Actually, it isn't as simple as that - this could happen if the compositor was created by the non-main window though I guess, so I still think overriding GetBounds might be a reasonable thing to do, or otherwise adding an Android-specific #ifdef if that causes bad behaviour.

Comment 19

[Chris Lord [:cwiiis]]

Oh if only it were as simple as I thought... Either way, pretty easy to reproduce, will track down.

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Set surface size on startup

So I had the right idea, but the wrong reasoning. It seems the compositor is always created at 0x0 at the moment, as there's (almost certainly) no chance that Gecko will process the resize event we send on startup before we create the compositor.

Calling a resume after creating with a size fixes this for me. This is the smallest fix, but perhaps we should alter the resumeComposition event to always take a size, which would involve fewer round-trips - I'm not sure if we'd want this or not, so feel free to r- and say that we do if you'd prefer that.

Comment 21

[Chris Lord [:cwiiis]]

So I implemented this other way of doing it (adding width/height properties to the resume event), and that doesn't fix this... So I think that webapps are ending up with a paused compositor? Investigating.

Comment 22

[Chris Lord [:cwiiis]]

Attachment: Proper fix for surface size mismatch/compositor pause on start

This applies over the previous patch and basically does the same thing, but with one less round-trip.

Ends up there were two probems;

1- The compositor has a 0x0 size on creation, and may not necessarily get a size again until the surface changes for an unrelated reason
2- nsWindow doesn't realise that after creation, the compositor is unpaused

Fixing these two fixes the issue in the same way as part 1, but with fewer round-trips.

Comment 24

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

Comment on attachment 727103 [details] [diff] [review]
Proper fix for surface size mismatch/compositor pause on start

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

::: mobile/android/base/gfx/GLController.java
@@ +159,5 @@
>              }
>          });
>      }
>  
> +    void createCompositor(int width, int height) {

This is unneeded; mWidth and mHeight will already have been set

@@ +164,4 @@
>          ThreadUtils.assertOnUiThread();
>  
> +        mWidth = width;
> +        mHeight = height;

This is unneeded

@@ +183,5 @@
>          }
>      }
>  
> +    void createCompositor() {
> +        createCompositor(mWidth, mHeight);

This is unneeded

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +142,5 @@
> +                // that the above resize event would have been handled before
> +                // this point, and otherwise the compositor surface size is
> +                // only updated on surface size changes and resumes, neither of
> +                // which are guaranteed to happen before the view is presented.
> +                mView.getGLController().createCompositor(mWindowSize.width, mWindowSize.height);

This change (and the previous patch) are unneeded.

::: widget/android/nsWindow.cpp
@@ +894,5 @@
> +                win->CreateLayerManager();
> +                sCompositorPaused = false;
> +
> +                win->mBounds.width = width;
> +                win->mBounds.height = height;

As discussed on IRC, I don't really like this code. I realize that we need a hack somewhere we interact with the window bounds because that code is pretty messed up, but I would prefer the hack to be a call to ScheduleResumeComposition which should have fewer (and better-understood) side effects.

::: widget/android/nsWindow.h
@@ +221,5 @@
>                               const nsIntPoint &refPoint);
>      void DispatchGestureEvent(uint32_t msg, uint32_t direction, double delta,
>                                const nsIntPoint &refPoint, uint64_t time);
>      void HandleSpecialKey(mozilla::AndroidGeckoEvent *ae);
> +    void CreateLayerManager();

Spurious change? Not sure why you moved this line.

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +864,5 @@
>    renderToEGLSurface = true;
>  #endif
>    nsIntRect rect;
>    GetBounds(rect);
> +  printf_stderr("Creating %dx%d compositor\n", rect.width, rect.height);

Take this out

Comment 25

[Chris Lord [:cwiiis]]

Attachment: Fix compositor creation

This creates a new create-compositor event that takes a width/height and makes sure the paused state is correct after creation. Benefit, creation no longer has an unnecessary RedrawAll and doesn't require a synchronous composite.

The confusing mBounds stuff has been removed and replaced by an extra vfunc on nsBaseWidget for creating a compositor with a size.

Comment 26

[Chris Lord [:cwiiis]]

Attachment: Fix compositor creation v2

kats noticed that I didn't add the new create event to the list of events that get moved to the front of the queue in nsAppShell.

Comment 27

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

Comment on attachment 727260 [details] [diff] [review]
Fix compositor creation v2

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

Much better, thanks! Just the one thing below that needs to be fixed, r=me with that, assuming try is all green.

::: widget/android/nsAppShell.cpp
@@ +648,5 @@
>              {
>                  uint32_t i = 0;
>                  while (i < mEventQueue.Length() &&
>                         (mEventQueue[i]->Type() == AndroidGeckoEvent::COMPOSITOR_PAUSE ||
>                          mEventQueue[i]->Type() == AndroidGeckoEvent::COMPOSITOR_RESUME)) {

This while condition also needs to be updated. I don't think the bad code path should get hit in practice but better to update it anyway.

Comment 28

[Chris Lord [:cwiiis]]

Thanks for the thorough review, this is a much nicer patch than the original versions - try was green even without the above mentioned fix, but fixed and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e70bea1fd166

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e70bea1fd166

Comment 30

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 23.0a1 (2013-04-08)
-device: Samsung Galaxy S II
-OS: Android 4.0.3