This bug was filed from the Socorro interface and is report bp-4f614847-26d5-4b87-9dff-3b8032140907. ============================================================= java.lang.AssertionError: null at org.mozilla.gecko.Assert.isTrue(Assert.java:37) at org.mozilla.gecko.overlays.ui.ShareDialog.onSendTabTargetSelected(ShareDialog.java:226) at org.mozilla.gecko.overlays.ui.SendTabDeviceListArrayAdapter$2.onClick(SendTabDeviceListArrayAdapter.java:128) at android.view.View.performClick(View.java:4475) at android.view.View$PerformClick.run(View.java:18786) at android.os.Handler.handleCallback(Handler.java:730) at android.os.Handler.dispatchMessage(Handler.java:92) at android.os.Looper.loop(Looper.java:137) at android.app.ActivityThread.main(ActivityThread.java:5419) at java.lang.reflect.Method.invokeNative(Native Method) at java.lang.reflect.Method.invoke(Method.java:525) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1209) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1025) at dalvik.system.NativeStart.main(Native Method) Looks like we have some

Tested with: Device: Samsung Galaxy Tab 3 (Android 4.4.2) Build: Firefox for Android 35.0a1 (2014-09-09) I can reproduce this crash with the following steps: (clean profile - sync is not set up) 1. Open a page 2. Choose Menu -> Share -> Add to nightly -> Send to another device 3. The "Welcome to Sync" message appears and set up sync 4. Tap "Back to browsing" 5. Go again to Menu -> Share -> Add to nightly -> Send to another device Nightly crashes.

Crash-stats says this is a startupcrash

Fennec doesn't need to be running when the share handler is invoked. That'll probably look like a startup crash.

I think what's happening is that you're beating the first sync, so we're in State.LOADING. That means we hit this: if (currentState == State.LIST) { row.setText(clientRecord.name); row.setCompoundDrawablesWithIntrinsicBounds(getImage(clientRecord), 0, 0, 0); listenerGUID = clientRecord.guid; } else { listenerGUID = null; } row.setOnClickListener(new OnClickListener() { @Override public void onClick(View view) { listener.onSendTabTargetSelected(listenerGUID); } }); I think that click listener should be inside the State.LIST clause.

Note that I can't reproduce this with a live account, even if I'm really really quick.

If I fake the state, then I can hit the assertion. Good enough for me.

I did a little cleanup while I was in the neighborhood. No warnings on these two files, now. The meat of the fix was to introduce a switch statement and move the logic for attaching a click handler. Now we don't attach a sending click handler unless this is a list of client records.

Comment on attachment 8505157 [details] [diff] [review] Avoid crash when Sync is partially configured. v1 Review of attachment 8505157 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/overlays/ui/SendTabDeviceListArrayAdapter.java @@ +105,5 @@ > + // Fall through. > + case LOADING: > + case NONE: > + // If we're in a special "Button-like" state, use the override string and a generic icon. > + row.setText(dummyRecordName); dummyRecordName won't be set for LOADING, so let me rev this patch.

APK for testing: http://people.mozilla.org/~rnewman/fennec/fx36-assert.apk

Trying my steps to reproduce in #comment1 with the apk provided: 1. Open google.com (sync is not set up) 2. Choose Menu -> Share -> Add to fennec -> Send to another device => nothing happens The "Welcome to Sync" message should appear, to set up sync, right?

Comment on attachment 8505175 [details] [diff] [review] Avoid crash when Sync is partially configured. v2 That implies there's something a little deeper going on here. Thanks for testing, Teodora.

Here's where I think the problem is: public void setSyncClients(final ParcelableClientRecord[] c) { final ParcelableClientRecord[] clients = c == null ? new ParcelableClientRecord[0] : c; int size = clients.length; if (size == 0) { // Just show a button to set up Sync (or whatever). switchState(NONE); return; } We set up Sync. We very quickly get to the share overlay. Sync is configured, so we roll on. But when we fetch the list of clients, we get an empty array, because we haven't fetched the clients collection yet. That puts us into this block, where we call switchState(NONE)... but we don't set an override intent, because we weren't in the failure start from the very beginning. The override intent mechanism is a little bit of a poor fit here, because there's state distributed around SendTab/SendTabList/SendTabDeviceListArrayAdapter. I'm seeing if I can get a minimal patch now, setting an override intent when we detect we have no clients. Otherwise I'll rip that out and do something simpler.

r=me.

I tested these changes with an account set up but not synced, and with a forced-empty clients list. In the former case I had a "Send tab to devices" row that took me to the status activity; in the latter case I had no send-tab-related rows at all.

APK refreshed if you want to revalidate.

Tested with: Device: Nexus 7 (Android 4.4.2) Build: apk provided With the steps provided in comment1 Firefox does not crash anymore, but I noticed some things while testing: 1. I installed the apk, set up sync, done some browsing and after that cleared the profile from android settings and removed the sync profile. After that I went to a page (google.com), chose Menu -> Share -> Add to fennec -> "Send to another device" and the pop-up is dismissed. I had to go one more time to Menu -> Share -> Add to fennec -> "Send to another device" so that the "Welcome to Sync" message appears. Reproducible every time I clear data and remove the sync profile. 2. After setting up sync and tapping the "Back to browsing" button, the focus should be on the page you openend at step 1, no? (for example here, google.com)

Comment on attachment 8505844 [details] [diff] [review] Part 1: avoid crash when Sync is partially configured. v2 Review of attachment 8505844 [details] [diff] [review]: ----------------------------------------------------------------- r+ 'cuz this should avoid the crash, but I don't think the model of the domain is rich enough. Explain to me why we're doing what appears to be a wrong thing in a reasonable case. ::: mobile/android/base/overlays/service/sharemethods/SendTab.java @@ +208,5 @@ > > + if (validGUIDs.isEmpty()) { > + // Guess we'd better override. We have no clients. > + // This does the broadcast for us. > + setOverrideIntent(FxAccountGetStartedActivity.class); This doesn't make sense. It's perfectly acceptable to have an empty set of remote clients and be well configured (i.e., in a state where launching Get Started would ping-pong through to Sync Settings). Is it not feasible to remove the Send Tab option(s) when we recognize this state? I.e., the handler of the "broadcastUIState" below does the right thing?

There are three possible actions: 1. Show "Send tab to devices", and when tapped display FxA to check status or set up Sync. 2. Show "Send tab to devices", and when tapped display a list of devices. 3. Show a list of devices. Our known states: A. We're not set up (zero accounts). NONE. => 1. B. We are set up, but action is required according to FxA. LOADING. => 1. C. We are set up, but we have no clients. => ???? D. We are set up, and have a few clients. => 3. E. We are set up, and have lots of clients. => 2. The state that this bug hits is C. We're not in state NONE or LOADING, so we don't queue up the intent to open FxA settings. We roll on into the C/D/E flow, which assumes we'll have at least one client to populate the row. We have none, so there are no GUIDs to pass along with the action. That triggers the "oh, must not be a single client" handling (for A, B, E), but that requires us to send along an intent to use instead. Without one we hit this assertion. The fix I implemented is to explicitly split out the "I'm an action" versus "I'm a client" calls (so one expects a GUID, and one expects an intent), and to send the FxA intent along for case C. I'm hoping that the situation Teodora is able to reproduce is simply that Sync hasn't synced yet (because FxA, token, info, meta, keys, clients takes a while), and the FxA status activity will be enough to show what's going on for affected users. Ideally we'd have a state "You have no other clients, dummy", but I'm happy to paper over this instead of working on UI that most people won't see. Does that analysis match up to your understanding, Nick?

Manual testing: * If I have Sync in a good state with lots of clients: I get expected behavior with the two-step list. * If I stub out a zero-client setup: public void setSyncClients(ParcelableClientRecord[] c) { c = null; /// For testing. we hit the assertion I just added: 10-16 19:40:56.244 E/GeckoAppShell( 6652): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main") 10-16 19:40:56.244 E/GeckoAppShell( 6652): java.lang.AssertionError: null 10-16 19:40:56.244 E/GeckoAppShell( 6652): at org.mozilla.gecko.Assert.isTrue(Assert.java:37) 10-16 19:40:56.244 E/GeckoAppShell( 6652): at org.mozilla.gecko.overlays.ui.ShareDialog.onSendTabActionSelected(ShareDialog.java:301) which isn't good. I think that's what Teodora hit (with real data) in her last test of my APK.

Removing the clause below that makes the zero-clients case work as expected -- i.e., show nothing. This will be confusing to some people, but hey.

This'll definitely need verifying.

I don't see why this is tracking 34; the feature only landed in 35.

Comment on attachment 8505844 [details] [diff] [review] Part 1: avoid crash when Sync is partially configured. v2 Approval Request Comment [Feature/regressing bug #]: Bug 948509, the share overlay, which is in 35. [User impact if declined]: Potential for crashes when trying to send a tab. [Describe test coverage new/current, TBPL]: Manual. [Risks and why]: It's a non-trivial change (still fairly small), but this has been well-exercised on Nightly. [String/UUID change made/needed]: None.

(In reply to Richard Newman [:rnewman] from comment #27) > I don't see why this is tracking 34; the feature only landed in 35. Oh! Because 34 used to be Aurora, and it was turned on at that point. E_INADEQUATE_FLAGS.

https://hg.mozilla.org/mozilla-aurora/rev/08509dd4569a https://hg.mozilla.org/mozilla-aurora/rev/edb00fd8a526