Split off from Bug 1081768. That bug is a particular instance of a class of bugs, which also includes Bug 1077858. The defining characteristic is that some resource -- the LayerView, BrowserDB -- are expected to be initialized by GeckoApp/BrowserApp long before they're used (by the compositor thread, by Telemetry:Gather), but due to Android lifecycle rules this expectation is incorrect. There are individual fixes and band-aids that we can apply: making the LayerView fetch block indefinitely (Bug 1081768), or eliminating BrowserDB altogether, allowing init to be driven by consumers (Bug 1077590, Bug 1080038), but the fundamental problem remains. I split this out because it stands a good chance of altering some of Fennec's lifecycle properties, and/or regressing startup, depending on where we measure.

With this patch, Fennec doesn't crash on first run.

Might or might not cause this (not a critical issue); I'll poke at this later. D/GeckoHealthRec(16898): Got all add-ons and prefs. I/GeckoHealthRec(16898): Persisting 0 add-ons. I/GeckoHealthRec(16898): Persisting prefs. D/GeckoHealthRec(16898): Incorporating environment: extensions.blocklist.enabled D/GeckoHealthRec(16898): Incorporating environment: intl.accept_languages D/GeckoHealthRec(16898): Incorporating environment: toolkit.telemetry.enabled E/GeckoHealthRec(16898): Exception handling message "HealthReport:Snapshot": E/GeckoHealthRec(16898): java.lang.IllegalStateException: Not initialized. E/GeckoHealthRec(16898): at org.mozilla.gecko.health.BrowserHealthRecorder.ensureEnvironment(BrowserHealthRecorder.java:298) E/GeckoHealthRec(16898): at org.mozilla.gecko.health.BrowserHealthRecorder.onEnvironmentChanged(BrowserHealthRecorder.java:276) E/GeckoHealthRec(16898): at org.mozilla.gecko.health.BrowserHealthRecorder.onEnvironmentChanged(BrowserHealthRecorder.java:256) E/GeckoHealthRec(16898): at org.mozilla.gecko.health.BrowserHealthRecorder.handleMessage(BrowserHealthRecorder.java:603) E/GeckoHealthRec(16898): at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:210) E/GeckoHealthRec(16898): at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:180) E/GeckoHealthRec(16898): at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2297) E/GeckoHealthRec(16898): at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method) E/GeckoHealthRec(16898): at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method) E/GeckoHealthRec(16898): at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method) E/GeckoHealthRec(16898): at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:411) E/GeckoHealthRec(16898): at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:190)

(In reply to Richard Newman [:rnewman] from comment #3) > E/GeckoHealthRec(16898): java.lang.IllegalStateException: Not initialized. This is simply due to Don't Keep Activities. BrowserHealthRecorder is tied to a GeckoApp instance; the snapshot message we get from Gecko is received by a newborn BHR. A separate bug. We do get this twice during startup, though: E/GeckoGLController(18255): eglCreateWindowSurface threw E/GeckoGLController(18255): java.lang.IllegalArgumentException: Make sure the SurfaceView or associated SurfaceHolder has a valid Surface E/GeckoGLController(18255): at com.google.android.gles_jni.EGLImpl._eglCreateWindowSurface(Native Method) E/GeckoGLController(18255): at com.google.android.gles_jni.EGLImpl.eglCreateWindowSurface(EGLImpl.java:92) E/GeckoGLController(18255): at org.mozilla.gecko.gfx.GLController.AttemptPreallocateEGLSurfaceForCompositor(GLController.java:292) E/GeckoGLController(18255): at org.mozilla.gecko.gfx.GLController.updateCompositor(GLController.java:181) E/GeckoGLController(18255): at org.mozilla.gecko.gfx.GLController$1.run(GLController.java:165) The comment in GLController explains what's going wrong here: // we defer to a runnable the task of updating the compositor, because this is going to // call back into createEGLSurfaceForCompositor, which will try to create an EGLSurface // against mView, which we suspect might fail if called too early. By posting this to // mView, we hope to ensure that it is deferred until mView is actually "ready" for some // sense of "ready". mView.post(new Runnable() { @Override public void run() { updateCompositor(); } });

Filed Bug 1085734 for the EGL stuff.

OK, I need some help with this one. This performs fine on the devices I've tested. I tested both tablet and phone, running KitKat, both first run and subsequent runs. Try shows that on 2.3 devices, nsIDOMWindowUtils.layerManagerType is failing, either because GetWidget is returning nothing, or GetLayerManager is returning nothing: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#2493 My guess is that one of three things is happening. * We're still showing the FRE during tests, the Gecko thread is kicked off so that the tests are starting to run, but there's some kind of race or rendering block such that the reftest setup code is running before we have layers. * We're hitting one of the edge cases in nsWindow.cpp that causes the wrong kind of (or no) layer manager to be created as a fallback. * The reftest harness is deeply, deeply racy, and I've managed to break it. Here's the Try push which shows the reftest failures: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f686f464c4d I just kicked off another, because there were some serious infra issues in that one, so many of the green tests were actually tests that never ran. I can't run reftests locally, because the instructions don't work, and nobody knows why. The only patch in that push moves the call to GeckoApp.initialize() into onStart(): https://hg.mozilla.org/try/rev/20d8faadbeb3 I chose to use onStart because: * initialize calls initializeChrome, which needs some setup to have happened at the end of BrowserApp.onCreate. If we try the end of GeckoApp.onCreate, we're not done setting up. * onResume didn't seem to work out in testing either, and it's pretty late in startup. So, folks: any ideas on either (a) how I can run reftests, (b) why this apparently simple change is borking the layer manager?

Took a quick look but it's unclear to me what's going on. Some suggestions: - Try to find a gingerbread device to try locally - GeckoApp.initialize() is huge. Try bisecting it to see which part of it is responsible for triggering the failure - Add more logging in the failing function call to see what's null and why that might be happening

(In reply to Richard Newman [:rnewman] from comment #9) > So, folks: any ideas on either (a) how I can run reftests? You may be hitting bug 1083279 -- try backing out 199775c34d69.

Comment on attachment 8508323 [details] [diff] [review] Initialize GeckoApp completely regardless of window focus events. v1 Review of attachment 8508323 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +1956,1 @@ > getWindow().setBackgroundDrawable(null); I wonder if we can just do this once in onCreate() and ditch the onWindowFocusChanged() altogether, though I sort of remember there being some visual regressions if this is done too early.... Alternatively, perhaps this can be moved to onResume() with initialize()?

Rather than continue to fight the rest of the system, I extracted the layer work up to the end of onCreate. That gets reftests passing locally; try is next. onPostCreate, alas, is posted after onStart, so it's too late for our purposes.

Here's the issue I've been wrestling with for way too long. On Try, on 2.3 emu, every reftest fails: REFTEST TEST-UNEXPECTED-FAIL | | EXCEPTION: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.layerManagerType]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://reftest/content/reftest.jsm :: BuildConditionSandbox :: line 609" data: no] I can't reproduce this on 2.3 hardware, on a 2.3 emulator launching Fennec, or even running reftests on the same virtual device image that the test infra is using. Neither can I see any reason why this would fail because of the reordering in this bug. It happens if I do *anything* to the ordering of operations -- earlier, later, split up. Doesn't matter. That implies that there's a fundamental coordination error somewhere in here. But I don't have the time to figure that out by running a new try build with more print statements every hour or two. Instead, I propose to use this fix: if we're on Android, catch a failure here and hard-code the only values that make sense. Here's the difference in logcat output between a real device and the failure: FAILS: I/GeckoLayerView( 424): XXX: getNativeWindow: org.mozilla.gecko.gfx.LayerView$LayerSurfaceView@406e1a68, null I/GeckoLayerView( 424): XXX: Holder: android.view.SurfaceView$3@4065d960 E/GeckoGLController( 424): eglCreateWindowSurface threw E/GeckoGLController( 424): java.lang.IllegalArgumentException: Make sure the SurfaceView or associated SurfaceHolder has a valid Surface E/GeckoGLController( 424): at com.google.android.gles_jni.EGLImpl._eglCreateWindowSurface(Native Method) E/GeckoGLController( 424): at com.google.android.gles_jni.EGLImpl.eglCreateWindowSurface(EGLImpl.java:87) E/GeckoGLController( 424): at org.mozilla.gecko.gfx.GLController.AttemptPreallocateEGLSurfaceForCompositor(GLController.java:294) E/GeckoGLController( 424): at org.mozilla.gecko.gfx.GLController.updateCompositor(GLController.java:183) E/GeckoGLController( 424): at org.mozilla.gecko.gfx.GLController$1.run(GLController.java:167) E/GeckoGLController( 424): at android.os.Handler.handleCallback(Handler.java:587) E/GeckoGLController( 424): at android.os.Handler.dispatchMessage(Handler.java:92) E/GeckoGLController( 424): at android.os.Looper.loop(Looper.java:130) E/GeckoGLController( 424): at android.app.ActivityThread.main(ActivityThread.java:3683) E/GeckoGLController( 424): at java.lang.reflect.Method.invokeNative(Native Method) E/GeckoGLController( 424): at java.lang.reflect.Method.invoke(Method.java:507) E/GeckoGLController( 424): at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:839) E/GeckoGLController( 424): at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:597) W/GeckoGLController( 424): eglCreateWindowSurface returned no surface! D/GeckoHardwareUtils( 424): HardwareUtils already inited. I/GeckoHealth( 424): fennec :: SubmissionPolicy :: Need to wait 86400000 before first upload. PASSES: I/GeckoLayerView( 616): XXX: getNativeWindow: org.mozilla.gecko.gfx.LayerView$LayerSurfaceView@40652938, null I/GeckoLayerView( 616): XXX: Holder: android.view.SurfaceView$3@40652d08 I/GeckoAppShell( 616): XXX getLayerView org.mozilla.gecko.GeckoView@405c3ba8 I/GeckoAppShell( 616): XXX getLayerView org.mozilla.gecko.GeckoView@405c3ba8 I/GeckoAppShell( 616): XXX getLayerView org.mozilla.gecko.GeckoView@405c3ba8 I/GeckoAppShell( 616): XXX getLayerView org.mozilla.gecko.GeckoView@405c3ba8 I/GeckoAppShell( 616): XXX getLayerView org.mozilla.gecko.GeckoView@405c3ba8 I/GeckoAppShell( 616): XXX getLayerView org.mozilla.gecko.GeckoView@405c3ba8 As you can see, the Java-side code has the same values, but the gfx guts evidently disagree. Flagging gbrown for review on this, even though it's reftests, because it's an Android-specific behavior.

Now making sure .Android is defined first, and with three fewer obvious errors.

Looks like it hides real issues: REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/text/line-editing-1a.html | image comparison (==), max difference: 255, number of differing pixels: 795 REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/text/line-editing-1b.html | image comparison (==), max difference: 255, number of differing pixels: 776 REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/text/line-editing-1c.html | image comparison (==), max difference: 255, number of differing pixels: 91 *tableflip*

Comment on attachment 8511088 [details] [diff] [review] Follow-up: make reftest harness resilient against layer issues. v2 Review of attachment 8511088 [details] [diff] [review]: ----------------------------------------------------------------- I'm okay with this in principle...just need to see that it works. :)

This looks relevant: http://stackoverflow.com/questions/12855103/eglcreatewindowsurface-fails-with-java-lang-illegalargumentexception

I've been digging into some gfx stuff. Back in Bug 736005 we *did* use GLSurfaceView, complete with the known-bad code path. But also, this is worrying: private class SurfaceListener implements SurfaceHolder.Callback { @Override public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) { onSizeChanged(width, height); } @Override public void surfaceCreated(SurfaceHolder holder) { } @Override public void surfaceDestroyed(SurfaceHolder holder) { onDestroyed(); } It's only valid to do surface stuff between surfaceCreated and surfaceDestroyed, so there might be a bug hiding here. kats, do you know why we don't use surfaceCreated? I can speculate (we're already waiting for Gecko to be ready before we call updateCompositor), but I'd love to know the reasoning! I think the situation we're having here is that we're not getting a surface because our activity hasn't received focus, so Gecko is ready before the surface (at least on emulated 2.3 on AWS). The fix is presumably to implement a barrier to only fire updateCompositor when both Gecko is ready and surfaceCreated has been called.

Some archaeology led me to https://bugzilla.mozilla.org/show_bug.cgi?id=745243#c8 which is the reasoning for why we wait for surfaceChanged instead of surfaceCreated. As for switching over from using GLSurface to SurfaceView, Jeff might have some answers based on the code shuffling I see in bug 740898 (in particular perhaps the "both styles of GL renderering" mentioned in comment 0 there might be important, in that it reflects a particular design we chose to go with).

Logging added for that push reveals that none of the SurfaceHolder.Callback callback methods are being called prior to AttemptPreallocate, and indeed they are never called before the app quits. By contrast, on 4.0 they are called. My assumption is that they *would* be called, they're just taking a while, and that's the problem; this logging supports my analysis in Comment 24. As far as I can tell, the current code assumes that we can roll on as soon as Gecko is ready, which is not the case.

My current best guess, based on research: Android doesn't create the surface unless it's visible on the screen. On 2.3. On the emulator. On AWS. Thinking about why that might be, and what to do about it.

(In reply to Richard Newman [:rnewman] from comment #24) (Sorry I had a response but looks like I didn't hit submit) We can bring up the compositor without an EGLSurface and we should be able to process layer transaction (receive update from content and send them off the to GPU) by using our context without any associated EGLSurface. However we need to make sure that we don't composite until we're received a surface. The compositor already supports a pause state so perhaps it should start in the pause state until it can associate an EGLSurface with the context.

Most of the current failures in bug 1054292 seem to happen when onWindowFocusChanged is not called.

I tried calling initialize() in onResume, but could not get a clean try run. Here's an alternative based on ViewTreeObserver.OnGlobalLayoutListener. Debug logging shows that this calls initialize() after onResume and before onWindowFocusChanged, but that may not be guaranteed. I had to keep the setFocusable calls in onWindowFocusChanged; otherwise some editor mochitests fail. https://treeherder.mozilla.org/#/jobs?repo=try&revision=21f9bd896041

Comment on attachment 8626537 [details] [diff] [review] an alternative using a global layout listener Martyn, throw this back at me if you need a second opinion.

Comment on attachment 8626537 [details] [diff] [review] an alternative using a global layout listener Review of attachment 8626537 [details] [diff] [review]: ----------------------------------------------------------------- I'll preface this by saying that I'm not an expert in this area of code - in fact, the more I dug in to it the more lost I got. It's a deep hole and I don't know all the implications of what this patch proposes. It feels good that we're removing the init method from onWindowFocusChanged and the new code looks good from a lifecycle point of view and looks like it'll run on the correct thread. Recommend waiting until 42 lands on nightly and then landing to see if there are any fallouts as a result.

(In reply to Martyn Haigh (:mhaigh) from comment #36) > Recommend waiting until 42 lands on nightly and then landing to see if there > are any fallouts as a result. Absolutely -- no need to rush.

Comment on attachment 8626537 [details] [diff] [review] an alternative using a global layout listener :jchen -- As discussed, would appreciate your thoughts here as well.

Comment on attachment 8626537 [details] [diff] [review] an alternative using a global layout listener Review of attachment 8626537 [details] [diff] [review]: ----------------------------------------------------------------- Logic looks good! I just have some nits. ::: mobile/android/base/GeckoApp.java @@ +1282,5 @@ > mMainLayout = (RelativeLayout) findViewById(R.id.main_layout); > mLayerView = (LayerView) findViewById(R.id.layer_view); > > + mMainLayout.getViewTreeObserver().addOnGlobalLayoutListener( > + new ViewTreeObserver.OnGlobalLayoutListener() { Can you make GeckoApp implement ViewTreeObserver.OnGlobalLayoutListener instead of using an anonymous class? @@ +1289,5 @@ > + if (Versions.preJB) { > + mMainLayout.getViewTreeObserver().removeGlobalOnLayoutListener(this); > + } else { > + mMainLayout.getViewTreeObserver().removeOnGlobalLayoutListener(this); > + } Move the remove listener call to before | if (!mInitialized) | block.

With changes for :jchen's nits - https://treeherder.mozilla.org/#/jobs?repo=try&revision=d64bf4cd3a05.

FWIW, I'm wondering how many null checks on chrome Views we can remove now that these Views are (more-or-less) guaranteed to be initialized by the time they're used.

Please request Aurora/Beta approval on this when you get a chance :)

I would prefer to see this change ride the trains: It feels high-risk.

Gecko 41 is 10 weeks from release and this fixed a pile of intermittent failures. Any chance I can convince you to at least consider taking it on Aurora?

Comment on attachment 8626537 [details] [diff] [review] an alternative using a global layout listener Approval Request Comment [Feature/regressing bug #]: This Java-only patch affects Fennec startup. A different Android mechanism is used to trigger initialization. [User impact if declined]: This change was motivated by intermittent test failures (startup hangs) observed on treeherder. This change may avoid intermittent startup hangs on physical devices, but I am unaware of any user-initiated bugs which have been attributed to this specific condition. [Describe test coverage new/current, TreeHerder]: I made no changes to tests. Of course most test suites start the browser -- lots of coverage! Startup-hang test failures were observed primarily in mochitest and robocop and have not been seen on m-c since landing. [Risks and why]: We may not fully understand the conditions governing the old and/or new Android notifications. If the new notification is not received, browser startup will not complete, leaving Fennec in a non-functional or poorly functioning state. Differences in notification timing may have unforeseen consequences. [String/UUID change made/needed]: none

Comment on attachment 8626537 [details] [diff] [review] an alternative using a global layout listener For now, let's take it in aurora only...