Bernd Ostermann Reporter
	Description • 12 years ago

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

Update Firefox to version 14.0 on Samsung Galay Nexus, Android 4.0.3;
open Firefox;
click a bookmark; 
or click history
or enter a search phrase


Actual results:

continues loading but never shows a result.


Expected results:

The requested bookmark or history entry or search results should be shown

	Aaron Train [:aaronmt]
	Comment 1 • 12 years ago

Can you attach a log of what happens when you attempt to visit a site? You can use aLogCat on Google Play or through the Android SDK (using logcat) to capture, save and attach to this bug.

	Bernd Ostermann Reporter
	Comment 2 • 12 years ago

Android DDMS LogCat file:
Actions:
Start FireFox V14.0 on Samsung Galaxy Nexus, Android V4.0.3;
click a bookmark entry
Repeated 2 times.

Result: selected bookmark result not displayed; Firefox hangs

	Aaron Train [:aaronmt]
	Comment 3 • 12 years ago

Just CC'ing graphics here to scan that log for anything obvious.

	Ali Juma [:ajuma]
	Comment 4 • 12 years ago

(In reply to Aaron Train [:aaronmt] from comment #3)
> Just CC'ing graphics here to scan that log for anything obvious.

Nothing suspicious jumps out. In fact, the log shows that we're successfully creating an OpenGL layer manager, ruling out things like GL context creation failure.

Perhaps this is something like the Looper-related problem in Bug 752153?

	Aaron Train [:aaronmt]
	Comment 5 • 12 years ago

https://support.mozilla.org/en-US/questions/921581

	Aaron Train [:aaronmt]
	Comment 6 • 12 years ago

 Unregistering receiver failed
06-28 17:59:51.350: E/GeckoConnectivityReceiver(8281): java.lang.IllegalArgumentException: Receiver not registered: org.mozilla.gecko.GeckoConnectivityReceiver@418613c8
06-28 17:59:51.350: E/GeckoConnectivityReceiver(8281): 	at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java:628)
06-28 17:59:51.350: E/GeckoConnectivityReceiver(8281): 	at android.app.ContextImpl.unregisterReceiver(ContextImpl.java:1066)
06-28 17:59:51.350: E/GeckoConnectivityReceiver(8281): 	at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:354)
06-28 17:59:51.350: E/GeckoConnectivityReceiver(8281): 	at org.mozilla.gecko.GeckoConnectivityReceiver.unregisterFor(GeckoConnectivityReceiver.java:98)

	rosends
	Comment 8 • 12 years ago

in addition, checking "About" causes a crash.

	Aaron Train [:aaronmt]
	Comment 9 • 12 years ago

Galaxy Nexus

https://play.google.com/store/apps/details?id=org.mozilla.firefox&reviewId=02154827881163398150
https://play.google.com/store/apps/details?id=org.mozilla.firefox&reviewId=10832581980073996864
https://play.google.com/store/apps/details?id=org.mozilla.firefox&reviewId=17280395127351814424

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 10 • 12 years ago

Need some more details:
* When tapping the menu, do you see the "Settings" enabled or disabled?
* Can you include more log? I'd like to see what happens during Firefox startup

	J
	Comment 11 • 12 years ago

I have this exact problem with ICS 4.0.4 on my Google Nexus S 4G (Sprint). Doesn't load anything, not even about:firefox. 

Android Version: 4.0.4
Build Number: IMM76D
Kernel: 3.0.8-g6656123

I tried uninstalling and reinstalling. Didn't do anything. Has nothing to do with addons, since they were all deleted when I uninstalled.

I tried the beta on the google app store. Just as broken as v14.0

v13 worked great. I really liked it, other than it's slowness.

Yesterday's nightly build sort of works.

It works fine on my friend's Google Nexus S (GSM on Tmobile). Build versions are identical. Only thing different is the baseband version for obvious reasons. 

----

* When tapping the menu, do you see the "Settings" enabled or disabled?

I could get to the menu without trouble.

* Can you include more log? I'd like to see what happens during Firefox startup

I reverted to v13.

	J
	Comment 12 • 12 years ago

https://support.mozilla.org/en-US/questions/930825

	Cyprian
	Comment 13 • 12 years ago

I faced same issue on my Samsung Galaxy S2 with Android 4.0.3. and Firefox 14

I'm not able to open any website from Awesomebar. 
When I try open Add-on's page I see only white screen.
Yesterday I was able to open a website from thumbnail on homepage (but only once per session), today it completely doesn't open any page.

That issue appeared after update from FF13 to FF14.

	Aaron Train [:aaronmt]
	Comment 14 • 12 years ago

J/Cyprian, can you attach logs (comment #1 how-to) capturing startup of Firefox 14?

	Brian Nicholson (:bnicholson)
	Comment 15 • 12 years ago

Here's a filtered version of the posted log, showing only Gecko messages: http://pastebin.mozilla.org/1684537

What's concerning is the number of "destroy" messages in the log, especially:

06-28 16:11:46.139: D/GeckoAwesomeBar(8281): creating awesomebar
06-28 16:11:46.154: I/GeckoViewsFactory(8281): Creating custom Gecko view: AwesomeBar$AwesomeBarEditText
06-28 16:11:46.162: I/GeckoViewsFactory(8281): Creating custom Gecko view: AwesomeBarTabs
06-28 16:11:46.162: D/GeckoAwesomeBarTabs(8281): Creating AwesomeBarTabs
06-28 16:11:46.170: D/GeckoAwesomeBarTabs(8281): Creating All Pages tab
06-28 16:11:46.178: D/GeckoAwesomeBarTabs(8281): Creating Bookmarks tab
06-28 16:11:46.186: D/GeckoAwesomeBarTabs(8281): Creating History tab
06-28 16:11:46.201: I/GeckoAwesomeBarTabs(8281): Got cursor in 12ms
>>> 06-28 16:11:47.115: I/GeckoApp(8281): stop <<<
>>> 06-28 16:11:47.123: I/GeckoApp(8281): destroy <<<

which appears twice in the log. This is evidence that GeckoApp (and therefore, Gecko itself) is getting killed when another activity - the AwesomeBar -  is launched.

To those experiencing this problem: do you have any task killer/memory management apps installed?

	Brian Nicholson (:bnicholson)
	Comment 16 • 12 years ago

(In reply to Brian Nicholson (:bnicholson) from comment #15)
> which appears twice in the log. This is evidence that GeckoApp (and
> therefore, Gecko itself) is getting killed when another activity - the
> AwesomeBar -  is launched.

Actually, I see this happen four times.

The first reply to bug https://support.mozilla.org/en-US/questions/930825 says the page loads if a predefined thumbnail is clicked (i.e., the AwesomeBar isn't opened), so this is likely the same issue.

	Cyprian
	Comment 17 • 12 years ago

that is what aLogcat shown:  http://pastebin.mozilla.org/1685048
I tried to open only one page google.com/reader

	Kevin Brosnan [Ex-Mozilla]
	Comment 18 • 12 years ago

I can reproduce this by setting 'Don't keep activities' in Android Settings > Developer Options. Firefox requires a background activity because of its hybrid Java/C++ design. Is there a way we can make sure both these activities are considered foreground? Should we just detect this case and require the user to switch the setting to use Firefox?

	Matt Brubeck (:mbrubeck)
	Comment 19 • 12 years ago

Can we add this to the release notes for 14.0 and 14.0.1?

	Aaron Train [:aaronmt]
	Comment 20 • 12 years ago

Nice find, hopefully it's the issue here. I wonder why people are doing toggling this setting in the first place.

	Jimstaffer
	Comment 21 • 12 years ago

I logged duplicate bug 769558 (Firefox 14.0 won't load any web pages on Android 4.0.3 Galaxy S2).

I can confirm that unselecting the "Don't keep activities", appears to fix this problem in FF 14.0 in my case. I have not tested FF 14.0 exhaustively, but it's loading pages now. I didn't know I had ticked this box or whether it was already ticked when I got the phone. I haven't even looked to see what it does yet! Thanks!

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 22 • 12 years ago

Could depend on bug 739789 as a possible solution

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 23 • 12 years ago

Is converting the Awesomebar screen to a popup from an activity enough? Does this still happen after visiting the Settings screen?

	Bernd Ostermann Reporter
	Comment 24 • 12 years ago

(In reply to Kevin Brosnan [:kbrosnan] from comment #18)
> I can reproduce this by setting 'Don't keep activities' in Android Settings
> > Developer Options. Firefox requires a background activity because of its
> hybrid Java/C++ design. Is there a way we can make sure both these
> activities are considered foreground? Should we just detect this case and
> require the user to switch the setting to use Firefox?

I think, that it is not a good idea to force the user to uncheck 'Don't keep activities'in developer options of settings. To check this option will always destroy an activity, when another activity is started. An activity may be destroyed in other situations e.g. when there is low memory. So unchecking 'Don't keep activities' will not solve the problem.
Perhaps it is a question, when a Receiver is registered and de-registered (s. logCat file). Should be in onResume and onPause.
Similiar problem in 
http://stackoverflow.com/questions/5036451/problem-with-broadcastreceiver-receiver-not-registered-error

	rosends
	Comment 25 • 12 years ago

I'm no expert, just an end user trying to provide documentation for the error i keep getting. galaxy nexus. I get no websites and a crash when i go into "about". I hope this isn't too much and is helpful. I haven't touched any developer stuff on my phone. i'm a not tech.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 26 • 12 years ago

rosends - can you look to see if the developer preference is somehow turned on anyway? Use this blog post to locate the setting: http://aaronmt.com/blog/2012/6/30/destroy-all-the-activities.html

It's part of Android's Settings, not Firefox.

	rosends
	Comment 27 • 12 years ago

there is a checkmark in that box. i don't know why -- i was cautioned not to touch anything in that area. should i uncheck it and try the software? what other implications will this have for my phone?

	Brian Nicholson (:bnicholson)
	Comment 28 • 12 years ago

(In reply to rosends from comment #27)
> there is a checkmark in that box. i don't know why -- i was cautioned not to
> touch anything in that area. should i uncheck it and try the software? what
> other implications will this have for my phone?

This option is disabled by default, so yes, you want it unchecked. Having this enabled will allow only one activity (http://developer.android.com/guide/components/activities.html) to be enabled at a time; it should be turned on only for testing purposes. With this option unchecked, you could see significant performance improvements on your phone.

	rosends
	Comment 29 • 12 years ago

thank you. so far, so good.

	Max Jonas Werner [:Makkes]
	Comment 31 • 12 years ago

Ahem, I stumbled upon the behaviour described here but don't have the "Don't keep activities" enabled (yes, I checked that several times).

The "Settings" option is greyed out in the Firefox menu.

	Bernd Ostermann Reporter
	Comment 32 • 12 years ago

(In reply to Max Jonas Werner from comment #31)
> Ahem, I stumbled upon the behaviour described here but don't have the "Don't
> keep activities" enabled (yes, I checked that several times).
> 
> The "Settings" option is greyed out in the Firefox menu.

"Don't keep activities" can be set in Settings, Developer options of the device, not of firefox. If "Don't keep activities" is unchecked, then Firefox will work correctly. It worked for me on Galayy Nexus, Android version 4.0.3.
But I think, that this is not the final solution. Firefox should work with "Don't keep activities" checked.

	Max Jonas Werner [:Makkes]
	Comment 33 • 12 years ago

I know where "Don't keep activities" can be enabled/disabled and I certainly disabled it in the developer settings of the phone.

That the "Settings" option is greyed out in the Firefox menu is an outcome of this bug I think.

	Matt Brubeck (:mbrubeck)
	Comment 34 • 12 years ago

If there is code in GeckoApp/BrowserApp that can't easily handle being destroyed and restarted while the process is still running, should we move it out of the Activity and into a Service instead?

	Matt Brubeck (:mbrubeck)
	Comment 35 • 12 years ago

(In reply to Max Jonas Werner from comment #33)
> I know where "Don't keep activities" can be enabled/disabled and I certainly
> disabled it in the developer settings of the phone.

It sounds like you're experiencing a different bug with similar symptoms.  Can you file a separate bug report (or if you filed one that was duplicated to this bug, reopen it) and attach a copy of your Android system log, please?  You can get the log using the "adb logcat" command from the Android SDK, or using an Android app like aLogCat from the Play Store.

	Max Jonas Werner [:Makkes]
	Comment 36 • 12 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #35)
> (In reply to Max Jonas Werner from comment #33)
> > I know where "Don't keep activities" can be enabled/disabled and I certainly
> > disabled it in the developer settings of the phone.
> 
> It sounds like you're experiencing a different bug with similar symptoms. 
> Can you file a separate bug report (or if you filed one that was duplicated
> to this bug, reopen it) and attach a copy of your Android system log,
> please?  You can get the log using the "adb logcat" command from the Android
> SDK, or using an Android app like aLogCat from the Play Store.

Damnit, after rebooting the phone Firefox does its job again. I'll have to wait until the buggy behaviour shows again before filing a bug.

	Kevin Brosnan [Ex-Mozilla]
	Comment 37 • 12 years ago

Did you have the setting checked at first? Then uncheked it? After switching the pref Firefox would need to be restarted so that Gecko can startup.

	Aaron Train [:aaronmt]
	Comment 39 • 12 years ago

Assignee? Patch?

	Brian Nicholson (:bnicholson)
	Comment 40 • 12 years ago

If we aren't able to get a proper solution in time, we can at least detect this and show a dialog, asking the user to change the setting.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 41 • 12 years ago

Comment on attachment 640402 [details] [diff] [review]
Show dialog when setting is enabled

Drive by 

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+        } else {
>+            AlertDialog.Builder builder = new AlertDialog.Builder(this);
>+            builder.setTitle(R.string.ics_dumbness_title);
>+            builder.setMessage(R.string.ics_dumbness_message);
>+            builder.setPositiveButton(R.string.button_ok, new DialogInterface.OnClickListener() {
>+                public void onClick(DialogInterface dialog, int which) {
>+                    final String ACTION = "android.intent.action.MAIN";
>+                    final String PACKAGE = "com.android.settings";
>+                    final String CLASS = PACKAGE + ".DevelopmentSettings";
>+                    Intent intent = new Intent(ACTION);
>+                    intent.setComponent(new ComponentName(PACKAGE, CLASS));
>+                    startActivity(intent);
>+                }

What happens if this is somehow triggered on non-ICS which does not have the "DevelopmentSettings" intent? Do we care? Will it cause a different kind of crash? Should we wrap this else block in a version check?

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY ics_dumbness_title "Problem detected">
>+<!ENTITY ics_dumbness_message "Firefox cannot be used if \&quot;Don\'t keep activities\&quot; is enabled. Please disable this option in the following screen.">

Can we drop the "in the following screen" bit?

	Brad Lassey [:blassey] (use needinfo?)
	Comment 42 • 12 years ago

Comment on attachment 640402 [details] [diff] [review]
Show dialog when setting is enabled

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

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +196,5 @@
>  <!ENTITY bookmarkhistory_import_history "Importing history
>                                           from Android">
>  <!ENTITY bookmarkhistory_import_wait "Please wait...">
> +
> +<!ENTITY ics_dumbness_title "Problem detected">

this sounds a bit awkward, but I can't really come up with a better description. Let's get a UX opinion before landing.

	Brian Nicholson (:bnicholson)
	Comment 43 • 12 years ago

(In reply to Mark Finkle (:mfinkle) from comment #41)
> What happens if this is somehow triggered on non-ICS which does not have the
> "DevelopmentSettings" intent? Do we care? Will it cause a different kind of
> crash? Should we wrap this else block in a version check?

I guess we might as well add the version check; it shouldn't hurt anything.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 44 • 12 years ago

Comment on attachment 640402 [details] [diff] [review]
Show dialog when setting is enabled

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY ics_dumbness_title "Problem detected">

Also, dialog captions use Title Case, so "Problem Detected"

	Brian Nicholson (:bnicholson)
	Comment 45 • 12 years ago

Updated from comments

	Brian Nicholson (:bnicholson)
	Comment 46 • 12 years ago

Rebased for beta

	Madhava Enros [:madhava]
	Comment 47 • 12 years ago

Worked through this with our illustrious copywriter, Matej.

Suggestion:


Title: There's a Problem
Body: "Don't keep activities" is turned on. Please turn it off in Settings > Developer options.
Buttons: [Cancel] [Settings]

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 48 • 12 years ago

Same as Brian's "v3" but with the new strings and the code for the [cancel] and [settings] buttons.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 49 • 12 years ago

and an APK is here:
http://people.mozilla.com/~mfinkle/fennec/fennec-there-is-a-problem.apk

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 50 • 12 years ago

Some WIP to try and fix this more. This patch applies on top of the two :bnchicolson gave me yesterday. With this page draw on initial render but then don't repaint when you pan/zoom around. Opening a second tab makes everything work great. I don't yet know why the drawing is partially busted when there's just one tab.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 51 • 12 years ago

Brian/Kats: Just and FYI. Bug 772510 changes some code related to mDoorhangerPopup which needs to be reverted when we make mDoorhangerPopup not a static.

	Aaron Train [:aaronmt]
	Comment 52 • 12 years ago

Ran through the APK in comment #49 via my Galaxy Nexus (4.1) and it works.

* Preference enabled
** am start -a android.intent.action.VIEW -n org.mozilla.fennec_mfinkle/.App http://mozilla.org
*** Dialog (dev option settings button works)

* Preference disabled
** am start -a android.intent.action.VIEW -n org.mozilla.fennec_mfinkle/.App http://mozilla.org
*** No dialog

Hopefully the right activity is started (com.android.settings/.Settings) on modified ICS versions?

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 53 • 12 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/58fc5e165c31

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 54 • 12 years ago

This is an updated patch which seems to fix the drawing issues. It's a full patch that includes :bnicholson's changes, and applies directly to m-c. The problem with the previous patch was because the awesome bar activity was getting destroyed after the geckoapp activity was created, and blassey's changes to GeckoApplication meant that the doc shell got left in an inactive state. So I undid blassey's changes to GeckoApplication and that fixed it (but might break other things). I'll try to split this patch up into more reasonable chunks.

	Brian Nicholson (:bnicholson)
	Comment 62 • 12 years ago

I think patches 3 and 4 actually came from blassey's patch (http://dump.lassey.us/ics_dumbness.patch).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 63 • 12 years ago

There's a build with these patches at https://people.mozilla.com/~kgupta/tmp/activity-restart.apk ("Fennec kats") for testing. I was just playing with it and it looks like bring up the settings page and then backing out results in a blank page. So there's still more work to be done here :(

	Brian Nicholson (:bnicholson)
	Comment 64 • 12 years ago

Comment on attachment 640721 [details] [diff] [review]
(7/7) bnicholson's Tab:Fetch code (may not be needed)

Using just patches 1-6, and I can confirm that this patch isn't needed.

	Brian Nicholson (:bnicholson)
	Comment 65 • 12 years ago

(In reply to Kartikaya Gupta (:kats) from comment #63)
> There's a build with these patches at
> https://people.mozilla.com/~kgupta/tmp/activity-restart.apk ("Fennec kats")
> for testing. I was just playing with it and it looks like bring up the
> settings page and then backing out results in a blank page. So there's still
> more work to be done here :(

This also happens for me if you just open the AwesomeBar and hit back without going to a URL.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 66 • 12 years ago

Updated this patch with an extra null check that I hit once in a while

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 67 • 12 years ago

Updated with a Viewport:Flush event that forces all the viewport info to get back in sync by pushing the browser.js/gecko viewport into java via setFirstPaintViewport

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 68 • 12 years ago

I've updated the build at https://people.mozilla.com/~kgupta/tmp/activity-restart.apk ("Fennec kats"), please test heavily.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 69 • 12 years ago

Update: something in part 7 (attachment #640721 [details] [diff] [review]) is needed for this to work; bnicholson will look into what it is. I've updated (again) the APK at the url in comment 68 to include part 7 so that it can be more thoroughly tested in the meantime.

	Brian Nicholson (:bnicholson)
	Comment 70 • 12 years ago

Added code to select tab when restoring.

	Aaron Train [:aaronmt]
	Comment 71 • 12 years ago

Marking 14 verified  (dialog/strings patch) on 14.0 Beta 12 (build #1) via same str in comment #52

Tested via Samsung Galaxy Nexus (Android 4.1) & Samsung Galaxy SII (Android 4.0)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 72 • 12 years ago

Comment on attachment 640717 [details] [diff] [review]
(3/7) bnicholson's patch to move the activity result handlers

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

This won't work for the case where the activity result handler used is not one of the 4 predefined ones. Starting an activity will blow away GeckoApp, which will blow away the ActivityResultHandlerMap, which will blow away the result handler stored in the mMap. When the activity finishes and we get the result back to a newly-created GeckoApp, it won't know which result handler to use to handle it.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 73 • 12 years ago

Comment on attachment 640719 [details] [diff] [review]
(5/7) Update graphics code to handle blowing away java objects and re-creating them

Not really sure who should review this, so asking ajuma and Cwiiis since they're probably the most familiar with this code? The gist of this patch is that since all the gfx/ java objects are blown away and re-created, we need to re-register the new GeckoLayerClient with the native code. This is accomplished by calling SetLayerClient, which also calls RegisterCompositor again to pick up the new GLController object. Note that ordinarily RegisterCompositor is called from the compositor thread, but in this case we are calling it from a java thread, so that's why we don't update the mThread or mJEnv objects in AndroidGLController::Reacquire.

	Chris Lord [:cwiiis]
	Comment 75 • 12 years ago

Comment on attachment 640719 [details] [diff] [review]
(5/7) Update graphics code to handle blowing away java objects and re-creating them

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

Looks pretty benign to me, but I think ajuma is more suitable to review. Changing my r? to an f+.

::: widget/android/AndroidLayerViewWrapper.cpp
@@ +54,5 @@
>  
> +void
> +AndroidGLController::Reacquire(JNIEnv *aJEnv, jobject aJObj)
> +{
> +    aJEnv->DeleteGlobalRef(mJObj);

Should this perhaps check mJObj isn't null first? Though I imagine if it is, we'd have crashed or something...

	Ali Juma [:ajuma]
	Comment 76 • 12 years ago

Comment on attachment 640719 [details] [diff] [review]
(5/7) Update graphics code to handle blowing away java objects and re-creating them

It seems possible for this to leave the compositor in a paused state, if the following sequence of events happens:

1) The compositor gets paused and existing java gfx objects get destroyed.
2) Java gfx objects get recreated.
3) The LayerView object receives a surfaceChanged call.
4) AndroidBridge::SetLayerClient is called.

Since the surfaceChanged happened before SetLayerClient (and hence before LayerView.registerCxxCompositor), GeckoLayerClient.surfaceChanged won't get called, so the compositor won't be told to resume. The compositor will remain paused until the next surfaceChanged (so, for example, until the device is rotated or the app is paused and resumed).

Note that the reason this doesn't affect us when the Java gfx objects are created at startup is that compositor creation includes waiting until a surface exists (by making a call to GLController.waitForValidSurface).

Here's an approach we can use. LayerView.registerCxxCompositor should be told when we're registering a pre-existing compositor. In this case, it should check whether we already have a valid surface (by making a call into the GLController object; we'll need to add a method to GLController that exposes this information), and if so, it should call compositionResumeRequested.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 77 • 12 years ago

(In reply to Ali Juma [:ajuma] from comment #76)
> Here's an approach we can use. LayerView.registerCxxCompositor should be
> told when we're registering a pre-existing compositor. In this case, it
> should check whether we already have a valid surface (by making a call into
> the GLController object; we'll need to add a method to GLController that
> exposes this information), and if so, it should call
> compositionResumeRequested.

I'm wondering if we should do this in all cases, not just when we're registering a pre-existing compositor. In effect this would eliminate the need to block on waitForValidSurface, thereby fixing bug 772672 as well. Instead of blocking and waiting, the compositor should just start up in a paused state if there is no surface available, and java can resume it once the surface is valid. Does that sound like it could work?

	Ali Juma [:ajuma]
	Comment 78 • 12 years ago

(In reply to Kartikaya Gupta (:kats) from comment #77)
> I'm wondering if we should do this in all cases, not just when we're
> registering a pre-existing compositor. In effect this would eliminate the
> need to block on waitForValidSurface, thereby fixing bug 772672 as well.
> Instead of blocking and waiting, the compositor should just start up in a
> paused state if there is no surface available, and java can resume it once
> the surface is valid. Does that sound like it could work?

The problem is that compositor creation (unlike resuming a pre-existing compositor) involves layer manager creation, which involves GLContext creation, which requires a surface (since otherwise we cannot make the context current).

We could abort compositor creation when there's no surface (rather than blocking), but making compositor creation abortable seems like a rather large change that we should avoid making unless we have to.

Probably a better solution to reduce blocking during compositor creation is to change nsWindow::OnDraw so that it checks if mLayerManager is null, and if so, calls into GLController to check if we have a valid surface, aborting the draw if we don't (which, in turn, prevents the creation of the compositor). Note that we already abort draws in certain situations (e.g. when the compositor is paused), so this is less risky change than making compositor creation abortable. Of course, there's still the small chance that we lose the surface between the check and the subsequent creation of the compositor, and hence end up blocking; nevertheless, this should eliminate most of the blocking.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 79 • 12 years ago

(In reply to Ali Juma [:ajuma] from comment #78)
> Probably a better solution to reduce blocking during compositor creation is
> to change nsWindow::OnDraw so that it checks if mLayerManager is null, and
> if so, calls into GLController to check if we have a valid surface, aborting
> the draw if we don't (which, in turn, prevents the creation of the
> compositor). Note that we already abort draws in certain situations (e.g.
> when the compositor is paused), so this is less risky change than making
> compositor creation abortable. Of course, there's still the small chance
> that we lose the surface between the check and the subsequent creation of
> the compositor, and hence end up blocking; nevertheless, this should
> eliminate most of the blocking.

Sounds reasonable. This can be done as part of bug 772672 then, since it's relatively independent of this bug.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 80 • 12 years ago

Comment on attachment 640715 [details] [diff] [review]
(1/7) Fix some logging

Moved this patch to bug 772855 along with a bunch of other similar changes.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 81 • 12 years ago

Rebased; carrying r+

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 82 • 12 years ago

Rebased, carrying r-

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 83 • 12 years ago

Comment on attachment 640784 [details] [diff] [review]
(4/7) bnicholson's patch to make things less static, with some updates by me (v2)

Remember to add mAppContext back to this change:
http://hg.mozilla.org/mozilla-central/rev/e6b770b14572

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 84 • 12 years ago

Comment on attachment 640814 [details] [diff] [review]
(6/7) bnicholson's patch to introduce mIsRestoring with graphics fixes (v3)

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+    private boolean mIsRestoring;

Let's call it mIsRestoringActivity since we also have a mRestoringSession. We are not consistent with our "Is" prefix too.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    Services.obs.addObserver(this, "Viewport:Flush", false);

Where is this coming from? I don't see it sent from anywhere.

r- just until we find "Viewport:Flush"

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 85 • 12 years ago

Rebased, carrying r+

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 86 • 12 years ago

Updated to address ajuma's comments. I put the call to resume the compositor in Reacquire because I figured it would be better to make sure the GLController object is updated in the JNI code first to prevent possible race conditions.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 87 • 12 years ago

Updated with comments. The Viewport:Flush event is sent inside the mIsRestoringActivity block of code.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 88 • 12 years ago

Comment on attachment 641537 [details] [diff] [review]
(5/5) Add an mIsRestoringActivity flag for restoring the activity

It was right there staring at me!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 89 • 12 years ago

I landed the four r+'d patches from this bug, since those will alleviate most of the problems, and have filed bug 773393 to address the issue from patch part 2, as it's turning into a more complex change.

Also marking this bug as [leave open] until 773393 is resolved.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4abd628b1fa1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ce3068cb1df
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd671e62713
https://hg.mozilla.org/integration/mozilla-inbound/rev/88dd29d90c52

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 90 • 12 years ago

Landed bug 773393 as well, so this bug can be closed when it gets merged to m-c.

	Ryan VanderMeulen [:RyanVM]
	Comment 91 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4abd628b1fa1
https://hg.mozilla.org/mozilla-central/rev/3ce3068cb1df
https://hg.mozilla.org/mozilla-central/rev/1dd671e62713
https://hg.mozilla.org/mozilla-central/rev/88dd29d90c52

	Brian Nicholson (:bnicholson)
	Comment 92 • 12 years ago

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: fennec doesn't work with "don't keep activities" on
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): ?
String or UUID changes made by this patch: none

This patch depends on bug 760152 and bug 752905.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 93 • 12 years ago

For those following along, we plan to wait a few days before uplifting this fix to Beta (Yes, Beta)

	Bernd Ostermann Reporter
	Comment 94 • 12 years ago

I have just installed the new version 14.0.1. Its release notes state, that this bug is fixed in 14.0.1. 
But to my surprise, only an error dialog is issued, which asks to unchek "Don't keep activities". The text of the dialog is a mixture of German and English, although the language was set to German.
That is no solution of the bug, a preliminary workaround at most.

	Matt Brubeck (:mbrubeck)
	Comment 95 • 12 years ago

(In reply to Bernd Ostermann from comment #94)
> I have just installed the new version 14.0.1. Its release notes state, that
> this bug is fixed in 14.0.1. 
> But to my surprise, only an error dialog is issued, which asks to unchek
> "Don't keep activities". The text of the dialog is a mixture of German and
> English, although the language was set to German.
> That is no solution of the bug, a preliminary workaround at most.

Yes, that was just a band-aid since the full solution was not complete and tested in time for the 14.0.1 release.  The full solution is currently in nightly builds, and we plan to ship it in the next update.

	Alex Keybl [:akeybl]
	Comment 96 • 12 years ago

Comment on attachment 642446 [details] [diff] [review]
"Don't keep activities" rollup patch, including bug 773393. r=mfinkle r=ajuma

[Triage Comment]
I acknowledge the unknown risk here, but we really want this fix, so let's start testing it asap. Approved for Beta 15.

	Brian Nicholson (:bnicholson)
	Comment 97 • 12 years ago

Landed rollup patch:

http://hg.mozilla.org/releases/mozilla-beta/rev/b2487714085b

	Cristian Nicolae (:xti)
	Comment 98 • 12 years ago

There is an improvement here, meaning that the pages are loading now, but there is a slow process. Loading another page in the same tab will take much longer until the page content will be displayed. Just the URL Title it's quickly displayed. Also the transition between Awesomescreen and the main view is laggy, and for ~1s the Awesomescreen color is displayed on the whole screen. 
As a conclusion, even if the processes are slower, is better than nothing, so the patch may be uplifted imo. However more testing is needed around this bug.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 99 • 12 years ago

The lagginess is totally expected. The UI is being recreated. It's the trade-off someone is making to reduce the amount of memory the phone is using.

We really need to be sure that no exceptions or crashes happen when using the "Don't keep activities" preference. As long as we are not crashing and the functionality is working, we have succeeded.

Thanks for testing this!

	Cristian Nicolae (:xti)
	Comment 100 • 12 years ago

(In reply to Mark Finkle (:mfinkle) from comment #99)
> The lagginess is totally expected. The UI is being recreated. It's the
> trade-off someone is making to reduce the amount of memory the phone is
> using.
> 
> We really need to be sure that no exceptions or crashes happen when using
> the "Don't keep activities" preference. As long as we are not crashing and
> the functionality is working, we have succeeded.
> 
> Thanks for testing this!

So far the app didn't crash, which is good, as you said. It happens just once that I couldn't access the Firefox Settings and the Tabs Menu as well, but I couldn't reproduce this issue. I will let the bug opened for now and I will close it after the patch will land on Aurora and Beta after testing both.

	Alex Keybl [:akeybl]
	Comment 101 • 12 years ago

Bug 760152 may need to be backed out to fix bug 763166. How difficult will it be to reland this without bug 760152 applied?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 102 • 12 years ago

I don't think this patch interacts with bug 760152 at all. Shouldn't be a problem.

	Alex Keybl [:akeybl]
	Comment 103 • 12 years ago

(In reply to Kartikaya Gupta (:kats) from comment #102)
> I don't think this patch interacts with bug 760152 at all. Shouldn't be a
> problem.

Thanks - this was w/r/t https://bugzilla.mozilla.org/show_bug.cgi?id=760152#c14.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 104 • 12 years ago

Ah, I missed one spot where this patch would have to be rebased without bug 760152. (Or rather, the backout of bug 760152 would need to be rebased on top of this patch). Still I don't think it would be a big problem, the dependency of mIsRestoringActivity on sGeckoThread can be broken easily by making it depend on some other static class variable.