The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
--
major
VERIFIED FIXED
5 years ago
8 months ago

People

(Reporter: Bernd Ostermann, Assigned: kats)

Tracking

(Depends on: 2 bugs, {relnote})

14 Branch
Firefox 16
ARM
Android
relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14+ verified, firefox15+ verified, firefox16 verified, firefox17 verified, blocking-fennec1.0 .N+)

Details

Attachments

(10 attachments, 15 obsolete attachments)

517.78 KB, text/plain
Details
95.50 KB, text/plain
Details
4.38 KB, patch
blassey
: review+
Details | Diff | Splinter Review
116.17 KB, image/png
Details
3.35 KB, patch
kats
: review+
kats
: checkin+
Details | Diff | Splinter Review
5.76 KB, patch
kats
: review-
Details | Diff | Splinter Review
17.34 KB, patch
kats
: review+
kats
: checkin+
Details | Diff | Splinter Review
11.15 KB, patch
ajuma
: review+
kats
: checkin+
Details | Diff | Splinter Review
5.31 KB, patch
mfinkle
: review+
kats
: checkin+
Details | Diff | Splinter Review
72.24 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 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
(Reporter)

Updated

5 years ago
Component: General → General
Product: Fennec Native → Firefox
Version: unspecified → 14 Branch

Updated

5 years ago
Component: General → General
Product: Firefox → Fennec Native
Version: 14 Branch → Firefox 14
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.
OS: Windows XP → Android
Hardware: x86 → ARM
(Reporter)

Comment 2

5 years ago
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
Just CC'ing graphics here to scan that log for anything obvious.

Comment 4

5 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?

Updated

5 years ago
Severity: normal → major
Status: UNCONFIRMED → NEW
blocking-fennec1.0: --- → ?
Ever confirmed: true
https://support.mozilla.org/en-US/questions/921581
 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)

Updated

5 years ago
status-firefox14: --- → affected

Updated

5 years ago
Duplicate of this bug: 769558

Comment 8

5 years ago
in addition, checking "About" causes a crash.
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
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

5 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.

Comment 12

5 years ago

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

Comment 13

5 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.
J/Cyprian, can you attach logs (comment #1 how-to) capturing startup of Firefox 14?
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?
(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

5 years ago
that is what aLogcat shown:  http://pastebin.mozilla.org/1685048
I tried to open only one page google.com/reader
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?
Can we add this to the release notes for 14.0 and 14.0.1?
Keywords: relnote
Summary: Firefox V 14.0 for Android loads but does not show any pages → Firefox does not load pages when "Don't keep activities" developer option is enabled in ICS
Nice find, hopefully it's the issue here. I wonder why people are doing toggling this setting in the first place.

Comment 21

5 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!
Could depend on bug 739789 as a possible solution
Depends on: 739789
Is converting the Awesomebar screen to a popup from an activity enough? Does this still happen after visiting the Settings screen?
(Reporter)

Comment 24

5 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

Comment 25

5 years ago
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.
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

5 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?
(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

5 years ago
thank you. so far, so good.

Updated

5 years ago
Duplicate of this bug: 770080
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.
(Reporter)

Comment 32

5 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.
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.
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?
(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.
(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.
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.
blocking-fennec1.0: ? → .N+

Updated

5 years ago
Duplicate of this bug: 771022

Updated

5 years ago
Summary: Firefox does not load pages when "Don't keep activities" developer option is enabled in ICS → Firefox does not load pages when "Don't keep activities" developer option is enabled in ICS/JB
Assignee? Patch?
status-firefox15: --- → affected
status-firefox16: --- → affected
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.
Attachment #640402 - Flags: review?(blassey.bugs)
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 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.
Attachment #640402 - Flags: review?(blassey.bugs) → review+
(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 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"
Created attachment 640490 [details] [diff] [review]
Show dialog when setting is enabled, v2

Updated from comments
Attachment #640402 - Attachment is obsolete: true
Attachment #640490 - Flags: review+
Created attachment 640494 [details] [diff] [review]
Show dialog when setting is enabled, v3

Rebased for beta
Attachment #640490 - Attachment is obsolete: true
Attachment #640494 - Flags: review+

Updated

5 years ago
tracking-firefox14: --- → +
tracking-firefox15: --- → +
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]
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.
Attachment #640494 - Attachment is obsolete: true
Attachment #640625 - Flags: review?(blassey.bugs)
Created attachment 640626 [details]
screenshot

and an APK is here:
http://people.mozilla.com/~mfinkle/fennec/fennec-there-is-a-problem.apk
Attachment #640625 - Flags: review?(blassey.bugs) → review+
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.
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.
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?
https://hg.mozilla.org/releases/mozilla-beta/rev/58fc5e165c31
status-firefox14: affected → fixed
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.
Attachment #640627 - Attachment is obsolete: true
Created attachment 640715 [details] [diff] [review]
(1/7) Fix some logging
Attachment #640715 - Flags: review?(mark.finkle)
Created attachment 640716 [details] [diff] [review]
(2/7) Fill in missing listener unregistrations
Attachment #640716 - Flags: review?(mark.finkle)
Created attachment 640717 [details] [diff] [review]
(3/7) bnicholson's patch to move the activity result handlers
Created attachment 640718 [details] [diff] [review]
(4/7) bnicholson's patch to make things less static, with some updates by me
Created attachment 640719 [details] [diff] [review]
(5/7) Update graphics code to handle blowing away java objects and re-creating them
Created attachment 640720 [details] [diff] [review]
(6/7) bnicholson's patch to introduce mIsRestoring
Created attachment 640721 [details] [diff] [review]
(7/7) bnicholson's Tab:Fetch code (may not be needed)
Attachment #640708 - Attachment is obsolete: true
I think patches 3 and 4 actually came from blassey's patch (http://dump.lassey.us/ics_dumbness.patch).
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 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.
Attachment #640721 - Attachment is obsolete: true
(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.
Attachment #640715 - Flags: review?(mark.finkle) → review+
Attachment #640716 - Flags: review?(mark.finkle) → review+
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
Attachment #640718 - Attachment is obsolete: true
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
Attachment #640720 - Attachment is obsolete: true
I've updated the build at https://people.mozilla.com/~kgupta/tmp/activity-restart.apk ("Fennec kats"), please test heavily.
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.
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.
Attachment #640786 - Attachment is obsolete: true
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)
status-firefox14: fixed → verified
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.
Attachment #640717 - Flags: review-
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.
Attachment #640719 - Flags: review?(chrislord.net)
Attachment #640719 - Flags: review?(ajuma)
Attachment #640784 - Flags: review?(mark.finkle)
Attachment #640814 - Flags: review?(mark.finkle)

Updated

5 years ago
Duplicate of this bug: 772707
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...
Attachment #640719 - Flags: review?(chrislord.net) → feedback+

Comment 76

5 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.
Attachment #640719 - Flags: review?(ajuma) → review-
(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

5 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.
(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 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.
Attachment #640715 - Attachment is obsolete: true
Created attachment 641512 [details] [diff] [review]
(1/5) Fill in missing listener unregistrations

Rebased; carrying r+
Attachment #640716 - Attachment is obsolete: true
Attachment #641512 - Flags: review+
Created attachment 641514 [details] [diff] [review]
(2/5) blassey's patch to move the activity result handlers

Rebased, carrying r-
Attachment #640717 - Attachment is obsolete: true
Attachment #641514 - Flags: review-
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
Attachment #640784 - Flags: review?(mark.finkle) → review+
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"
Attachment #640814 - Flags: review?(mark.finkle) → review-
Created attachment 641534 [details] [diff] [review]
(3/5) blassey/bnicholson's patch to make things less static

Rebased, carrying r+
Attachment #640784 - Attachment is obsolete: true
Attachment #641534 - Flags: review+
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.
Attachment #640719 - Attachment is obsolete: true
Attachment #641536 - Flags: review?(ajuma)
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.
Attachment #640814 - Attachment is obsolete: true
Attachment #641537 - Flags: review?(mark.finkle)

Updated

5 years ago
Attachment #641536 - Flags: review?(ajuma) → review+
Comment on attachment 641537 [details] [diff] [review]
(5/5) Add an mIsRestoringActivity flag for restoring the activity

It was right there staring at me!
Attachment #641537 - Flags: review?(mark.finkle) → review+
Depends on: 773393
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
Whiteboard: [leave open]
Assignee: nobody → bugmail.mozilla
Attachment #641512 - Flags: checkin+
Attachment #641534 - Flags: checkin+
Attachment #641536 - Flags: checkin+
Attachment #641537 - Flags: checkin+
Landed bug 773393 as well, so this bug can be closed when it gets merged to m-c.
Whiteboard: [leave open]
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16

Updated

5 years ago
status-firefox16: affected → fixed
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.
Attachment #642446 - Flags: approval-mozilla-aurora?
Depends on: 774156
For those following along, we plan to wait a few days before uplifting this fix to Beta (Yes, Beta)

Updated

5 years ago
Attachment #642446 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(Reporter)

Comment 94

5 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.
(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 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.
Attachment #642446 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Landed rollup patch:

http://hg.mozilla.org/releases/mozilla-beta/rev/b2487714085b
status-firefox15: affected → fixed
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.
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!
(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.
Depends on: 774205

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
status-firefox16: fixed → verified
status-firefox17: --- → verified
Depends on: 777505
Bug 760152 may need to be backed out to fix bug 763166. How difficult will it be to reland this without bug 760152 applied?
I don't think this patch interacts with bug 760152 at all. Shouldn't be a problem.
(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.
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.
Depends on: 780699
Depends on: 781883
Depends on: 781729
You need to log in before you can comment on or make changes to this bug.