Tested with: Device: Nexus 5 (Android 4.4) Builds: Firefox for Android 35.0a1 (2014-09-24) and Firefox for Android 34.0a2 (2014-09-24) Steps to reproduce: 1. Enable "Don't keep activities" from Developer Tools 2. Open Firefox Expected results: - Firefox opens correctly Actual results: - Firefox becomes unresponsive: "Unfortunately, Firefox has stopped"

Looks like BrowserApp is being killed because the Startup activity is being shown. We are throwing an exception because we are trying to unregister an event that did not get a chance to even be registered. We could: 1. move the registration calls to be earlier, 2. not throw exceptions in unregister, or 3. look at making the StartPane be a fragment and not a separate activity. I/ActivityManager( 786): Start proc org.mozilla.fennec for activity org.mozilla.fennec/.App: pid=4639 uid=10074 gids={50074, 3003, 1028, 1015} I/ActivityManager( 786): Killing 4247:com.google.android.gms.drive/u0a7 (adj 15): empty #17 I/ActivityManager( 786): START u0 {cmp=org.mozilla.fennec/org.mozilla.gecko.StartPane} from pid 4639 E/AndroidRuntime( 4639): FATAL EXCEPTION: main E/AndroidRuntime( 4639): Process: org.mozilla.fennec, PID: 4639 E/AndroidRuntime( 4639): java.lang.IllegalArgumentException: Gecko:Ready was not registered E/AndroidRuntime( 4639): at org.mozilla.gecko.EventDispatcher.unregisterListener(EventDispatcher.java:102) E/AndroidRuntime( 4639): at org.mozilla.gecko.EventDispatcher.unregisterGeckoThreadListener(EventDispatcher.java:140) E/AndroidRuntime( 4639): at org.mozilla.gecko.GeckoApp.onDestroy(GeckoApp.java:1986) E/AndroidRuntime( 4639): at org.mozilla.gecko.BrowserApp.onDestroy(BrowserApp.java:1110) E/AndroidRuntime( 4639): at android.app.Activity.performDestroy(Activity.java:5403)

[Tracking Requested - why for this release]:regession

Let's try to make this a fragment or popupwindow

Since first-run experience was introduced in 23/08 build until 17/09 build, trying to tap "Sign in to Nightly" or "Start Browsing", a message appears "Nightly isn't responding. Do you want to close it?". Since 18/09 build, the message appears: "Unfortunately, Nightly has stopped".

(In reply to Mark Finkle (:mfinkle) from comment #1) > We could: 1. move the registration calls to be earlier, 2. not throw > exceptions in unregister, or 3. look at making the StartPane be a fragment > and not a separate activity. I think we should probably do (1) and/or (2) -- or a safer version thereof -- regardless of the specific fix we tackle for this manifestation. Given that we don't have absolute control over our lifecycle, we shouldn't fail hard when our overly high expectations aren't met! I also tagged Bug 900799 as a dep, 'cos that would have spotted this issue prior to landing.

This patch turns the StartPane into a DialogFragment, which can be launched by BrowserApp.

The problem is not really that StartPane needs to be a Fragment - we can have activities that are separate from BrowserApp (Settings, for example). But like we discussed in the frontend meeting, the problem is that killing BrowserApp/Gecko during initialization causes us to try to tear down listeners, etc that we haven't actually set up. I don't think that turning StartPane into a Fragment is the right approach, because the v2 of the Onboarding is going to be much more complex than just a dialog - we want to allow different (swipeable) screens for importing from other browsers, introducing Firefox differentiators, etc. However, for a fix for the v1, I think the solution is to turn this into a DialogFragment.

Note that these patches build applied on top of the patch from bug 1059792.

Further to this discussion: I agree that DialogFragment is the right way to go here. I didn't have enough context in the FE meeting today, and after reading some more I think mfinkle is absolutely correct: there's no path to multiple Activity's using Gecko (or even asking it to start in the background!) that doesn't involve a huge re-engineering to stop managing Gecko's lifecycle in the Activity lifecycle. I'll comment in Bug 1077583 about what such re-engineering might look like.

(In reply to Nick Alexander :nalexander from comment #10) > there's no path to > multiple Activity's using Gecko (or even asking it to start in the > background!) that doesn't involve a huge re-engineering to stop managing > Gecko's lifecycle in the Activity lifecycle. That's not what we're talking about, though. We're talking about this: is it possible for another activity to safely come to the foreground before GeckoApp has finished initializing? Not multiple activities using Gecko, just ordinary lifecycle overlaps. Fixing GeckoApp to be a safe singleTask activity is much smaller in scope than decoupling GeckoApp from Gecko.

(In reply to Richard Newman [:rnewman] from comment #11) > (In reply to Nick Alexander :nalexander from comment #10) > > there's no path to > > multiple Activity's using Gecko (or even asking it to start in the > > background!) that doesn't involve a huge re-engineering to stop managing > > Gecko's lifecycle in the Activity lifecycle. > > That's not what we're talking about, though. > > We're talking about this: is it possible for another activity to safely come > to the foreground before GeckoApp has finished initializing? > > Not multiple activities using Gecko, just ordinary lifecycle overlaps. > > Fixing GeckoApp to be a safe singleTask activity is much smaller in scope > than decoupling GeckoApp from Gecko. I think I agree, but suppose we did this, and made this work. Is it more than hygiene? We want to start Gecko when the first-run activity starts. With "don't keep Activities", this cannot be done in any way that makes sense to me while GeckoApp owns Gecko's lifecycle. So is this valuable?

(In reply to Nick Alexander :nalexander from comment #12) > I think I agree, but suppose we did this, and made this work. Is it more > than hygiene? We want to start Gecko when the first-run activity starts. > With "don't keep Activities", this cannot be done in any way that makes > sense to me while GeckoApp owns Gecko's lifecycle. So is this valuable? GeckoApp doesn't really own Gecko's lifecycle; that's what GeckoThread does. We're actually already part-way to having the browser "attach" to Gecko correctly -- I think we do indeed do so, if the disconnection (onDestroy) happens at the right time. We just aren't very good at hygiene around listeners,, initialization, and coordinating between JS and Java-side state (e.g., SharedPreferences vs. per-profile Gecko prefs) during the rocky startup/shutdown phases. That's the bit I think we can fix, perhaps by extending some of the explicit state management in GeckoThread to analogues in GeckoApp. (We're a long way from having multiple activities using a single Gecko thread, which of course is a dramatically more complicated problem.)

Comment on attachment 8499679 [details] [diff] [review] Patch: Use DialogFragment Review of attachment 8499679 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just giving f+ to get more context about this checkStartPane() call. ::: mobile/android/base/BrowserApp.java @@ +667,5 @@ > // If we're restarting, we won't destroy the activity. Make sure we remove any guest notifications that might have been shown. > GuestSession.hideNotification(this); > } > + > + checkStartPane(this, getIntent().getAction()); Where is this implemented? I couldn't find it anywhere. ::: mobile/android/base/StartPane.java @@ +61,4 @@ > } > > // Add handler for dismissing the StartPane on a single click. > + private void addDismissHandler(View view) { Wondering: do we currently dismiss the startpane when tapping outside the dialog content area? If not, we probably should i.e. something like calling getDialog().setCanceledOnTouchOutside(true) in onCreateView(). File a follow-up if this is necessary. @@ +61,5 @@ > } > > // Add handler for dismissing the StartPane on a single click. > + private void addDismissHandler(View view) { > + final GestureDetector gestureDetector = new GestureDetector(getActivity(), new GestureDetector.SimpleOnGestureListener() { This could have been a simple View.OnClickListener(), no? @@ +66,3 @@ > @Override > public boolean onSingleTapUp(MotionEvent e) { > + StartPane.this.getDialog().cancel(); You better call StartPane.this.dismiss() here. ::: mobile/android/base/resources/values/styles.xml @@ +815,5 @@ > <item name="android:windowIsTranslucent">true</item> > <item name="android:backgroundDimEnabled">true</item> > </style> > <style name="OnboardStartLayout"> > + <item name="android:windowFullscreen">true</item> I assume there's no need to explicitly set windowIsFloating=false or something?

> Looks good, just giving f+ to get more context about this checkStartPane() > call. > Ah yeah, I was hunting down a rc failure in that bug, so it hadn't landed yet - see bug 1059792. > > ::: mobile/android/base/StartPane.java > @@ +61,4 @@ > > } > > > > // Add handler for dismissing the StartPane on a single click. > > + private void addDismissHandler(View view) { > > Wondering: do we currently dismiss the startpane when tapping outside the > dialog content area? If not, we probably should i.e. something like calling > getDialog().setCanceledOnTouchOutside(true) in onCreateView(). File a > follow-up if this is necessary. > I used DialogFragment to make things easier so I didn't have to manage the Fragments manually, so this is actually a dialog that is fullscreen and still matches the mocks in that it looks like an activity. > @@ +61,5 @@ > > } > > > > // Add handler for dismissing the StartPane on a single click. > > + private void addDismissHandler(View view) { > > + final GestureDetector gestureDetector = new GestureDetector(getActivity(), new GestureDetector.SimpleOnGestureListener() { > > This could have been a simple View.OnClickListener(), no? > I tried just adding a click listener, but I think the child views were consuming the clicks, so I used a GestureHandler which allowed the scrollview to handle the clicks. I can look into using an OnClickListener in a follow-up though, if you'd prefer that simplicity. > @@ +66,3 @@ > > @Override > > public boolean onSingleTapUp(MotionEvent e) { > > + StartPane.this.getDialog().cancel(); > > You better call StartPane.this.dismiss() here. Thanks, done. > > ::: mobile/android/base/resources/values/styles.xml > @@ +815,5 @@ > > <item name="android:windowIsTranslucent">true</item> > > <item name="android:backgroundDimEnabled">true</item> > > </style> > > <style name="OnboardStartLayout"> > > + <item name="android:windowFullscreen">true</item> > > I assume there's no need to explicitly set windowIsFloating=false or > something? Nope, since this ends up being fullscreen, the IsFloating doesn't seem to come into play - I tried adding it just to see, and nothing was different. I tried this out on a 2.3 device, however, which I hadn't done before because 2.3 devices don't have the "Don't keep activities" setting, and I realized that loading a Fragment is slower on those devices, so we get a glimpse of the BrowserToolbar UI before the Onboarding UI appears. :/ I looked at putting the checkStartPane code into various parts of the Activity (onResume, onPostResume, OnPostCreate, etc) but those are actually too early in the Gecko lifecycle and cause crashes because some platform code isn't initialized yet or something. http://pastebin.mozilla.org/6739991 I'm going to look into delaying displaying the BrowserToolbar UI until the StartPane check happens tomorrow - do you have other suggestions?

(In reply to Chenxia Liu [:liuche] from comment #15) > Created attachment 8501527 [details] [diff] [review] > Patch: Use DialogFragment v2 > > > > Looks good, just giving f+ to get more context about this checkStartPane() > > call. > > > > Ah yeah, I was hunting down a rc failure in that bug, so it hadn't landed > yet - see bug 1059792. Got it, thanks. > > > > ::: mobile/android/base/StartPane.java > > @@ +61,4 @@ > > > } > > > > > > // Add handler for dismissing the StartPane on a single click. > > > + private void addDismissHandler(View view) { > > > > Wondering: do we currently dismiss the startpane when tapping outside the > > dialog content area? If not, we probably should i.e. something like calling > > getDialog().setCanceledOnTouchOutside(true) in onCreateView(). File a > > follow-up if this is necessary. > > > > I used DialogFragment to make things easier so I didn't have to manage the > Fragments manually, so this is actually a dialog that is fullscreen and > still matches the mocks in that it looks like an activity. Are we going to show a fullscreen dialog on tablets too? > > @@ +61,5 @@ > > > } > > > > > > // Add handler for dismissing the StartPane on a single click. > > > + private void addDismissHandler(View view) { > > > + final GestureDetector gestureDetector = new GestureDetector(getActivity(), new GestureDetector.SimpleOnGestureListener() { > > > > This could have been a simple View.OnClickListener(), no? > > > > I tried just adding a click listener, but I think the child views were > consuming the clicks, so I used a GestureHandler which allowed the > scrollview to handle the clicks. I can look into using an OnClickListener in > a follow-up though, if you'd prefer that simplicity. Not a big deal. Strictly speaking, the right thing to do here would be to override onInterceptTouchEvent() in the dialog's toplevel container. You can file a follow-up. > > @@ +66,3 @@ > > > @Override > > > public boolean onSingleTapUp(MotionEvent e) { > > > + StartPane.this.getDialog().cancel(); > > > > You better call StartPane.this.dismiss() here. > > Thanks, done. > > > > > ::: mobile/android/base/resources/values/styles.xml > > @@ +815,5 @@ > > > <item name="android:windowIsTranslucent">true</item> > > > <item name="android:backgroundDimEnabled">true</item> > > > </style> > > > <style name="OnboardStartLayout"> > > > + <item name="android:windowFullscreen">true</item> > > > > I assume there's no need to explicitly set windowIsFloating=false or > > something? > > Nope, since this ends up being fullscreen, the IsFloating doesn't seem to > come into play - I tried adding it just to see, and nothing was different. > > I tried this out on a 2.3 device, however, which I hadn't done before > because 2.3 devices don't have the "Don't keep activities" setting, and I > realized that loading a Fragment is slower on those devices, so we get a > glimpse of the BrowserToolbar UI before the Onboarding UI appears. :/ > > I looked at putting the checkStartPane code into various parts of the > Activity (onResume, onPostResume, OnPostCreate, etc) but those are actually > too early in the Gecko lifecycle and cause crashes because some platform > code isn't initialized yet or something. > > http://pastebin.mozilla.org/6739991 > > I'm going to look into delaying displaying the BrowserToolbar UI until the > StartPane check happens tomorrow - do you have other suggestions? I was wondering about this yesterday i.e. whether or not the browser UI should show up before the fragment is displayed. If we're giving up on the idea of showing the start pane as a dialog, maybe the start pane should be an activity that runs *before* BrowserApp? This is more of long-term idea though. For now, you can try hiding the UI until you know whether or not the startpane will be displayed. But this might cause problems in the perceived startup time.

Comment on attachment 8501527 [details] [diff] [review] Patch: Use DialogFragment v2 Review of attachment 8501527 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/StartPane.java @@ +34,5 @@ > + public void onClick(View v) { > + Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.BUTTON, "firstrun-sync"); > + > + // StartPane is on the stack above the browser, so just dismiss this Fragment. > + StartPane.this.getDialog().dismiss(); StartPane.this.dismiss() @@ +66,3 @@ > @Override > public boolean onSingleTapUp(MotionEvent e) { > + StartPane.this.getDialog().dismiss(); StartPane.this.dismiss() ::: mobile/android/base/resources/values/styles.xml @@ +816,5 @@ > <item name="android:windowIsTranslucent">true</item> > <item name="android:backgroundDimEnabled">true</item> > </style> > <style name="OnboardStartLayout"> > + <item name="android:windowFullscreen">true</item> It's fullscreen on both phones and tablets then?

So this is patch hides the startup UI so that it doesn't flash when you launch the start pane. A few things: - I originally wanted to just toggle mBrowserToolbar or mBrowserChrome, but for some reason, I'd still see them flash momentarily before the StartPane was shown. I'm not really sure why this is because I wasn't seeing any of the setVisibility() calls for those views in BrowserApp being called. Toggling mActionBarFlipper seemed to work, though. - Unfortunately, this approach might require some further hacks because there isn't really a reliable place to unhide the startup UI for fast/slow deviecs. I looked at unhiding the UI in onStart or onResume of the DialogFragment, but launching the Dialog is so slow on some devices, the startup UI gets unhidden before the Dialog pops up. I stuck the unhiding code in onDismiss, but there, on some devices, the mActionBarFlipper view doesn't become visible fast enough. Anyways, trying to find the right place to wait for Gecko to finish initializing so we can launch the DialogFragment without seeing the browser UI seems to be really tricky for all devices because Gecko takes different amounts of time to start on 2.3 vs other devices :(

(In reply to Lucas Rocha (:lucasr) from comment #17) > Comment on attachment 8501527 [details] [diff] [review] > Patch: Use DialogFragment v2 > > Review of attachment 8501527 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/resources/values/styles.xml > @@ +816,5 @@ > > <item name="android:windowIsTranslucent">true</item> > > <item name="android:backgroundDimEnabled">true</item> > > </style> > > <style name="OnboardStartLayout"> > > + <item name="android:windowFullscreen">true</item> > > It's fullscreen on both phones and tablets then? No, this is just for phones - we have a values-v11-large/styles file where OnboardStartLayout is not fullscreen (and in Dialog format).

Comment on attachment 8502240 [details] [diff] [review] Part 2: Hide startup UI Removing feedback request based on our conversation yesterday.

Dropping my patch that makes a Dialog in here, but as rnewman mentioned on irc, it looks like bug 1085591 fixes this, and I'd prefer taking that very-small-clean-patch to hacking a DialogFragment in.

...and by this, I mean the crash when using "Don't keep activities" due to Gecko not following through with initialization, which prompted the use of DialogFragment instead of an Activity.

Comment on attachment 8508364 [details] [diff] [review] Patch: Dialog Requesting review in case the reftest bugsplosion of bug 1085591 doesn't get worked out for uplift. Turns onboarding pane into a Dialog.

Comment on attachment 8508364 [details] [diff] [review] Patch: Dialog Review of attachment 8508364 [details] [diff] [review]: ----------------------------------------------------------------- Ok. Maybe share screenshots for antlam's review?

Anthony, here's a screenshot of the dialog on phones, which is no longer full-screen, but a floating dialog. This is so that we don't crash for certain devices when "Don't keep activities" is enabled. If bug 1085591 lands soon, we won't need to land or uplift this, and we can also keep to the original mocks, but there are complications with reftests, so this is a fallback.

I think the root of the reftest failures is actually a core gfx lifecycle issue. Even if that only affects older Android versions running on emulators, it's risky enough that I wouldn't be keen on uplifting it to 34. We should probably expect this workaround to go out in 34, even if the real fix is done soon and uplifted to 35, unless there is a strong experience reason why the dialog approach isn't suitable.

Comment on attachment 8512130 [details] Screenshot: Phone dialog Trying to pick up this work now with the help of more insights in the UR side with Gemma. This looks like a good start to fix some of those issues raised in the bugs. R+ing to get this moving :)

Approval Request Comment [Feature/regressing bug #]: Onboarding work [User impact if declined]: Fennec will crash on some devices with "Don't keep activities" enabled [Describe test coverage new/current, TBPL]: local, fx-team try [Risks and why]: low, switch to Fragment lifecycle instead of Activity [String/UUID change made/needed]: none

Triage drive-by: waiting for successful landing to central before approving aurora uplift.

Comment on attachment 8512307 [details] [diff] [review] Aurora patch: Dialog [Triage Comment] Also approving for Beta uplift since this is tracked for 34.

Green beta run. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=57718ae6ed68 Please use *this* patch for beta uplift, not the aurora patch.

https://hg.mozilla.org/releases/mozilla-aurora/rev/996306da489f https://hg.mozilla.org/releases/mozilla-beta/rev/79567465c505

Verified as fixed in Firefox for Android 35 Beta 1 and Firefox for Android 34 with Nexus 4 (Android 4.4.4).