769269 - Firefox does not load pages when "Don't keep activities" developer option is enabled in ICS/JB

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Bernd Ostermann]

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]]

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]

Attachment: 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]]

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

Comment 4

[Ali Juma [:ajuma]]

(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]]

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

Comment 6

[Aaron Train [:aaronmt]]

 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 8

[rosends]

in addition, checking "About" causes a crash.

Comment 9

[Aaron Train [:aaronmt]]

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

Comment 10

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

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]

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 12

[J]

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

Comment 13

[Cyprian]

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]]

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

Comment 15

[Brian Nicholson (:bnicholson)]

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)]

(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]

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

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

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)]

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

Comment 20

[Aaron Train [:aaronmt]]

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

Comment 21

[Jimstaffer]

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?)]

Could depend on bug 739789 as a possible solution

Comment 23

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

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]

(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]

Attachment: 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?)]

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]

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)]

(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]

thank you. so far, so good.

Comment 31

[Max Jonas Werner [:Makkes]]

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]

(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]]

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)]

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)]

(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]]

(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 [Ex-Mozilla]]

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 39

[Aaron Train [:aaronmt]]

Assignee? Patch?

Comment 40

[Brian Nicholson (:bnicholson)]

Attachment: 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?)]

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?)]

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)]

(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?)]

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)]

Attachment: Show dialog when setting is enabled, v2

Updated from comments

Comment 46

[Brian Nicholson (:bnicholson)]

Attachment: Show dialog when setting is enabled, v3

Rebased for beta

Comment 47

[Madhava Enros [:madhava]]

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?)]

Attachment: 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?)]

Attachment: screenshot

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

Comment 50

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

Attachment: 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?)]

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]]

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?)]

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

Comment 54

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

Attachment: 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

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

Attachment: (1/7) Fix some logging

Comment 56

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

Attachment: (2/7) Fill in missing listener unregistrations

Comment 57

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

Attachment: (3/7) bnicholson's patch to move the activity result handlers

Comment 58

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

Attachment: (4/7) bnicholson's patch to make things less static, with some updates by me

Comment 59

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

Attachment: (5/7) Update graphics code to handle blowing away java objects and re-creating them

Comment 60

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

Attachment: (6/7) bnicholson's patch to introduce mIsRestoring

Comment 61

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

Attachment: (7/7) bnicholson's Tab:Fetch code (may not be needed)

Comment 62

[Brian Nicholson (:bnicholson)]

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

Comment 63

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

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)]

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)]

(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

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

Attachment: (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

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

Attachment: (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

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

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

Comment 69

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

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)]

Attachment: (6/7) bnicholson's patch to introduce mIsRestoring with graphics fixes (v3)

Added code to select tab when restoring.

Comment 71

[Aaron Train [:aaronmt]]

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

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

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

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

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 75

[Chris Lord [:cwiiis]]

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]]

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

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

(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]]

(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

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

(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

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

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

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

Attachment: (1/5) Fill in missing listener unregistrations

Rebased; carrying r+

Comment 82

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

Attachment: (2/5) blassey's patch to move the activity result handlers

Rebased, carrying r-

Comment 83

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

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?)]

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

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

Attachment: (3/5) blassey/bnicholson's patch to make things less static

Rebased, carrying r+

Comment 86

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

Attachment: (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

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

Attachment: (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?)]

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

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

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

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

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

Comment 91

[Ryan VanderMeulen [:RyanVM]]

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

Comment 92

[Brian Nicholson (:bnicholson)]

Attachment: "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?)]

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

Comment 94

[Bernd Ostermann]

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)]

(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]]

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)]

Landed rollup patch:

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

Comment 98

[Cristian Nicolae (:xti)]

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?)]

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)]

(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]]

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

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

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

Comment 103

[Alex Keybl [:akeybl]]

(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

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

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.