Last Comment Bug 769269 - Firefox does not load pages when "Don't keep activities" developer option is enabled in ICS/JB
: Firefox does not load pages when "Don't keep activities" developer option is ...
Status: VERIFIED FIXED
: relnote
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 14 Branch
: ARM Android
: -- major with 1 vote (vote)
: Firefox 16
Assigned To: (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
: 769558 770080 771022 772707 (view as bug list)
Depends on: 739789 781729 773393 774156 774205 777505 780699 781883
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-28 07:25 PDT by Bernd Ostermann
Modified: 2012-08-13 08:54 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
verified
verified
.N+


Attachments
Logcat file: starting FF V14.0, bookmark clicked (517.78 KB, text/plain)
2012-06-28 09:17 PDT, Bernd Ostermann
no flags Details
Fromt he download to the install to running, trying a google search, clicking settings, about and the crash (95.50 KB, text/plain)
2012-07-01 09:13 PDT, rosends
no flags Details
Show dialog when setting is enabled (4.03 KB, patch)
2012-07-09 16:08 PDT, Brian Nicholson (:bnicholson)
blassey.bugs: review+
Details | Diff | Review
Show dialog when setting is enabled, v2 (4.07 KB, patch)
2012-07-09 20:19 PDT, Brian Nicholson (:bnicholson)
bnicholson: review+
Details | Diff | Review
Show dialog when setting is enabled, v3 (4.07 KB, patch)
2012-07-09 21:00 PDT, Brian Nicholson (:bnicholson)
bnicholson: review+
Details | Diff | Review
patch with ux strings (4.38 KB, patch)
2012-07-10 09:09 PDT, Mark Finkle (:mfinkle) (use needinfo?)
blassey.bugs: review+
Details | Diff | Review
screenshot (116.17 KB, image/png)
2012-07-10 09:10 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details
WIP to fix drawing (20.15 KB, patch)
2012-07-10 09:13 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP Patch (37.78 KB, patch)
2012-07-10 11:59 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
(1/7) Fix some logging (1.12 KB, patch)
2012-07-10 12:23 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
(2/7) Fill in missing listener unregistrations (3.35 KB, patch)
2012-07-10 12:24 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
(3/7) bnicholson's patch to move the activity result handlers (5.76 KB, patch)
2012-07-10 12:24 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review-
Details | Diff | Review
(4/7) bnicholson's patch to make things less static, with some updates by me (16.10 KB, patch)
2012-07-10 12:25 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
(5/7) Update graphics code to handle blowing away java objects and re-creating them (7.46 KB, patch)
2012-07-10 12:26 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
ajuma.bugzilla: review-
chrislord.net: feedback+
Details | Diff | Review
(6/7) bnicholson's patch to introduce mIsRestoring (2.75 KB, patch)
2012-07-10 12:26 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
(7/7) bnicholson's Tab:Fetch code (may not be needed) (3.83 KB, patch)
2012-07-10 12:27 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
(4/7) bnicholson's patch to make things less static, with some updates by me (v2) (16.46 KB, patch)
2012-07-10 14:23 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
(6/7) bnicholson's patch to introduce mIsRestoring with graphics fixes (4.97 KB, patch)
2012-07-10 14:25 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
(6/7) bnicholson's patch to introduce mIsRestoring with graphics fixes (v3) (5.12 KB, patch)
2012-07-10 15:11 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review-
Details | Diff | Review
(1/5) Fill in missing listener unregistrations (3.35 KB, patch)
2012-07-12 09:57 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review+
bugmail.mozilla: checkin+
Details | Diff | Review
(2/5) blassey's patch to move the activity result handlers (5.76 KB, patch)
2012-07-12 09:58 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review-
Details | Diff | Review
(3/5) blassey/bnicholson's patch to make things less static (17.34 KB, patch)
2012-07-12 11:03 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review+
bugmail.mozilla: checkin+
Details | Diff | Review
(4/5) Update graphics code to handle blowing away Java objects (v2) (11.15 KB, patch)
2012-07-12 11:05 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
ajuma.bugzilla: review+
bugmail.mozilla: checkin+
Details | Diff | Review
(5/5) Add an mIsRestoringActivity flag for restoring the activity (5.31 KB, patch)
2012-07-12 11:08 PDT, (Back on May31) Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
bugmail.mozilla: checkin+
Details | Diff | Review
"Don't keep activities" rollup patch, including bug 773393. r=mfinkle r=ajuma (72.24 KB, patch)
2012-07-15 16:41 PDT, Brian Nicholson (:bnicholson)
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Bernd Ostermann 2012-06-28 07:25:00 PDT
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
Comment 1 Aaron Train [:aaronmt] 2012-06-28 07:36:29 PDT
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.
Comment 2 Bernd Ostermann 2012-06-28 09:17:30 PDT
Created attachment 637541 [details]
Logcat file: starting FF V14.0, bookmark clicked

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
Comment 3 Aaron Train [:aaronmt] 2012-06-28 09:26:23 PDT
Just CC'ing graphics here to scan that log for anything obvious.
Comment 4 Ali Juma [:ajuma] 2012-06-29 07:14:46 PDT
(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?
Comment 5 Aaron Train [:aaronmt] 2012-06-29 08:05:55 PDT
https://support.mozilla.org/en-US/questions/921581
Comment 6 Aaron Train [:aaronmt] 2012-06-29 08:06:51 PDT
 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)
Comment 7 Aaron Train [:aaronmt] 2012-06-29 08:09:10 PDT
*** Bug 769558 has been marked as a duplicate of this bug. ***
Comment 8 rosends 2012-06-29 10:21:35 PDT
in addition, checking "About" causes a crash.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-29 11:56:18 PDT
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
Comment 11 J 2012-06-29 15:19:25 PDT
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.
Comment 13 Cyprian 2012-06-29 16:12:42 PDT
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.
Comment 14 Aaron Train [:aaronmt] 2012-06-29 16:20:10 PDT
J/Cyprian, can you attach logs (comment #1 how-to) capturing startup of Firefox 14?
Comment 15 Brian Nicholson (:bnicholson) 2012-06-29 16:41:58 PDT
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?
Comment 16 Brian Nicholson (:bnicholson) 2012-06-29 16:46:55 PDT
(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.
Comment 17 Cyprian 2012-06-30 00:18:03 PDT
that is what aLogcat shown:  http://pastebin.mozilla.org/1685048
I tried to open only one page google.com/reader
Comment 18 Kevin Brosnan [:kbrosnan] 2012-06-30 09:48:08 PDT
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?
Comment 19 Matt Brubeck (:mbrubeck) 2012-06-30 10:05:57 PDT
Can we add this to the release notes for 14.0 and 14.0.1?
Comment 20 Aaron Train [:aaronmt] 2012-06-30 12:17:52 PDT
Nice find, hopefully it's the issue here. I wonder why people are doing toggling this setting in the first place.
Comment 21 Jimstaffer 2012-06-30 16:41:38 PDT
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!
Comment 22 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-30 21:19:37 PDT
Could depend on bug 739789 as a possible solution
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-30 21:47:05 PDT
Is converting the Awesomebar screen to a popup from an activity enough? Does this still happen after visiting the Settings screen?
Comment 24 Bernd Ostermann 2012-07-01 06:46:02 PDT
(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
Comment 25 rosends 2012-07-01 09:13:51 PDT
Created attachment 638193 [details]
Fromt he download to the install to running, trying a google search, clicking settings, about and the crash

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.
Comment 26 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-01 10:48:14 PDT
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.
Comment 27 rosends 2012-07-01 10:51:06 PDT
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?
Comment 28 Brian Nicholson (:bnicholson) 2012-07-01 11:51:05 PDT
(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.
Comment 29 rosends 2012-07-01 11:56:48 PDT
thank you. so far, so good.
Comment 30 Aaron Train [:aaronmt] 2012-07-01 21:55:38 PDT
*** Bug 770080 has been marked as a duplicate of this bug. ***
Comment 31 Max Jonas Werner [:Makkes] 2012-07-03 01:11:15 PDT
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.
Comment 32 Bernd Ostermann 2012-07-03 04:09:59 PDT
(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.
Comment 33 Max Jonas Werner [:Makkes] 2012-07-03 04:30:12 PDT
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.
Comment 34 Matt Brubeck (:mbrubeck) 2012-07-03 07:37:51 PDT
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?
Comment 35 Matt Brubeck (:mbrubeck) 2012-07-03 07:39:43 PDT
(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.
Comment 36 Max Jonas Werner [:Makkes] 2012-07-03 08:45:55 PDT
(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.
Comment 37 Kevin Brosnan [:kbrosnan] 2012-07-03 09:05:47 PDT
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.
Comment 38 Aaron Train [:aaronmt] 2012-07-04 16:09:06 PDT
*** Bug 771022 has been marked as a duplicate of this bug. ***
Comment 39 Aaron Train [:aaronmt] 2012-07-09 06:55:30 PDT
Assignee? Patch?
Comment 40 Brian Nicholson (:bnicholson) 2012-07-09 16:08:40 PDT
Created attachment 640402 [details] [diff] [review]
Show dialog when setting is enabled

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.
Comment 41 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-09 17:59:23 PDT
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?
Comment 42 Brad Lassey [:blassey] (use needinfo?) 2012-07-09 18:03:08 PDT
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.
Comment 43 Brian Nicholson (:bnicholson) 2012-07-09 18:08:21 PDT
(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.
Comment 44 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-09 18:12:14 PDT
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"
Comment 45 Brian Nicholson (:bnicholson) 2012-07-09 20:19:36 PDT
Created attachment 640490 [details] [diff] [review]
Show dialog when setting is enabled, v2

Updated from comments
Comment 46 Brian Nicholson (:bnicholson) 2012-07-09 21:00:56 PDT
Created attachment 640494 [details] [diff] [review]
Show dialog when setting is enabled, v3

Rebased for beta
Comment 47 Madhava Enros [:madhava] 2012-07-10 07:53:31 PDT
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]
Comment 48 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-10 09:09:19 PDT
Created attachment 640625 [details] [diff] [review]
patch with ux strings

Same as Brian's "v3" but with the new strings and the code for the [cancel] and [settings] buttons.
Comment 49 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-10 09:10:20 PDT
Created attachment 640626 [details]
screenshot

and an APK is here:
http://people.mozilla.com/~mfinkle/fennec/fennec-there-is-a-problem.apk
Comment 50 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 09:13:43 PDT
Created attachment 640627 [details] [diff] [review]
WIP to fix drawing

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.
Comment 51 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-10 09:36:07 PDT
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.
Comment 52 Aaron Train [:aaronmt] 2012-07-10 10:04:53 PDT
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?
Comment 53 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-10 10:06:23 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/58fc5e165c31
Comment 54 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 11:59:08 PDT
Created attachment 640708 [details] [diff] [review]
WIP Patch

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.
Comment 55 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:23:33 PDT
Created attachment 640715 [details] [diff] [review]
(1/7) Fix some logging
Comment 56 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:24:10 PDT
Created attachment 640716 [details] [diff] [review]
(2/7) Fill in missing listener unregistrations
Comment 57 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:24:44 PDT
Created attachment 640717 [details] [diff] [review]
(3/7) bnicholson's patch to move the activity result handlers
Comment 58 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:25:23 PDT
Created attachment 640718 [details] [diff] [review]
(4/7) bnicholson's patch to make things less static, with some updates by me
Comment 59 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:26:09 PDT
Created attachment 640719 [details] [diff] [review]
(5/7) Update graphics code to handle blowing away java objects and re-creating them
Comment 60 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:26:51 PDT
Created attachment 640720 [details] [diff] [review]
(6/7) bnicholson's patch to introduce mIsRestoring
Comment 61 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:27:24 PDT
Created attachment 640721 [details] [diff] [review]
(7/7) bnicholson's Tab:Fetch code (may not be needed)
Comment 62 Brian Nicholson (:bnicholson) 2012-07-10 12:34:09 PDT
I think patches 3 and 4 actually came from blassey's patch (http://dump.lassey.us/ics_dumbness.patch).
Comment 63 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 12:38:03 PDT
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 :(
Comment 64 Brian Nicholson (:bnicholson) 2012-07-10 13:06:36 PDT
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.
Comment 65 Brian Nicholson (:bnicholson) 2012-07-10 13:14:50 PDT
(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.
Comment 66 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 14:23:38 PDT
Created attachment 640784 [details] [diff] [review]
(4/7) bnicholson's patch to make things less static, with some updates by me (v2)

Updated this patch with an extra null check that I hit once in a while
Comment 67 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 14:25:10 PDT
Created attachment 640786 [details] [diff] [review]
(6/7) bnicholson's patch to introduce mIsRestoring with graphics fixes

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
Comment 68 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 14:27:28 PDT
I've updated the build at https://people.mozilla.com/~kgupta/tmp/activity-restart.apk ("Fennec kats"), please test heavily.
Comment 69 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-10 14:39:12 PDT
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.
Comment 70 Brian Nicholson (:bnicholson) 2012-07-10 15:11:14 PDT
Created attachment 640814 [details] [diff] [review]
(6/7) bnicholson's patch to introduce mIsRestoring with graphics fixes (v3)

Added code to select tab when restoring.
Comment 71 Aaron Train [:aaronmt] 2012-07-10 16:09:34 PDT
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)
Comment 72 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-11 06:43:14 PDT
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.
Comment 73 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-11 06:52:09 PDT
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.
Comment 74 Aaron Train [:aaronmt] 2012-07-11 06:58:36 PDT
*** Bug 772707 has been marked as a duplicate of this bug. ***
Comment 75 Chris Lord [:cwiiis] 2012-07-11 07:02:22 PDT
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...
Comment 76 Ali Juma [:ajuma] 2012-07-11 08:35:21 PDT
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.
Comment 77 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-11 09:11:37 PDT
(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?
Comment 78 Ali Juma [:ajuma] 2012-07-11 12:16:46 PDT
(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.
Comment 79 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-11 12:35:23 PDT
(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.
Comment 80 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 07:27:08 PDT
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.
Comment 81 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 09:57:00 PDT
Created attachment 641512 [details] [diff] [review]
(1/5) Fill in missing listener unregistrations

Rebased; carrying r+
Comment 82 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 09:58:02 PDT
Created attachment 641514 [details] [diff] [review]
(2/5) blassey's patch to move the activity result handlers

Rebased, carrying r-
Comment 83 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-12 10:06:18 PDT
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
Comment 84 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-12 10:13:36 PDT
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"
Comment 85 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 11:03:55 PDT
Created attachment 641534 [details] [diff] [review]
(3/5) blassey/bnicholson's patch to make things less static

Rebased, carrying r+
Comment 86 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 11:05:53 PDT
Created attachment 641536 [details] [diff] [review]
(4/5) Update graphics code to handle blowing away Java objects (v2)

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.
Comment 87 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 11:08:16 PDT
Created attachment 641537 [details] [diff] [review]
(5/5) Add an mIsRestoringActivity flag for restoring the activity

Updated with comments. The Viewport:Flush event is sent inside the mIsRestoringActivity block of code.
Comment 88 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-12 11:51:32 PDT
Comment on attachment 641537 [details] [diff] [review]
(5/5) Add an mIsRestoringActivity flag for restoring the activity

It was right there staring at me!
Comment 89 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-13 07:25:57 PDT
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
Comment 90 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-13 11:52:56 PDT
Landed bug 773393 as well, so this bug can be closed when it gets merged to m-c.
Comment 92 Brian Nicholson (:bnicholson) 2012-07-15 16:41:19 PDT
Created attachment 642446 [details] [diff] [review]
"Don't keep activities" rollup patch, including bug 773393. r=mfinkle r=ajuma

[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.
Comment 93 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-16 06:23:01 PDT
For those following along, we plan to wait a few days before uplifting this fix to Beta (Yes, Beta)
Comment 94 Bernd Ostermann 2012-07-17 10:47:00 PDT
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.
Comment 95 Matt Brubeck (:mbrubeck) 2012-07-17 10:48:35 PDT
(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.
Comment 96 Alex Keybl [:akeybl] 2012-07-18 16:34:29 PDT
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.
Comment 97 Brian Nicholson (:bnicholson) 2012-07-18 21:10:38 PDT
Landed rollup patch:

http://hg.mozilla.org/releases/mozilla-beta/rev/b2487714085b
Comment 98 Cristian Nicolae (:xti) 2012-07-19 08:12:37 PDT
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.
Comment 99 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-19 08:18:44 PDT
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!
Comment 100 Cristian Nicolae (:xti) 2012-07-19 08:30:36 PDT
(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.
Comment 101 Alex Keybl [:akeybl] 2012-07-30 16:23:04 PDT
Bug 760152 may need to be backed out to fix bug 763166. How difficult will it be to reland this without bug 760152 applied?
Comment 102 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-30 17:36:50 PDT
I don't think this patch interacts with bug 760152 at all. Shouldn't be a problem.
Comment 103 Alex Keybl [:akeybl] 2012-07-31 09:56:44 PDT
(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.
Comment 104 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-31 10:10:33 PDT
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.

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