crash in java.lang.AssertionError: null at org.mozilla.gecko.Assert.isTrue(Assert.java)

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
Overlays
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: aaronmt, Assigned: rnewman)

Tracking

({crash, reproducible})

35 Branch
Firefox 36
All
Android
crash, reproducible
Points:
---
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox34 unaffected, firefox35 fixed, firefox36 fixed, fennec35+)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Keywords: reproducible
(Reporter)

Comment 2

4 years ago
Crash-stats says this is a startupcrash
(Assignee)

Comment 3

4 years ago
Fennec doesn't need to be running when the share handler is invoked. That'll probably look like a startup crash.
tracking-fennec: ? → 34+
(Reporter)

Updated

4 years ago
Status: NEW → ASSIGNED
Assignee: chriskitching → rnewman
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
Note that I can't reproduce this with a live account, even if I'm really really quick.
(Assignee)

Comment 6

3 years ago
If I fake the state, then I can hit the assertion. Good enough for me.
(Assignee)

Comment 7

3 years ago
Created attachment 8505157 [details] [diff] [review]
Avoid crash when Sync is partially configured. v1

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.
Attachment #8505157 - Flags: review?(nalexander)
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
APK for testing:

http://people.mozilla.org/~rnewman/fennec/fx36-assert.apk
Flags: needinfo?(teodora.vermesan)
(Assignee)

Comment 10

3 years ago
Created attachment 8505175 [details] [diff] [review]
Avoid crash when Sync is partially configured. v2
Attachment #8505175 - Flags: review?(nalexander)
(Assignee)

Updated

3 years ago
Attachment #8505157 - Attachment is obsolete: true
Attachment #8505157 - Flags: review?(nalexander)
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?
Flags: needinfo?(teodora.vermesan)
(Assignee)

Comment 12

3 years ago
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.
Attachment #8505175 - Flags: review?(nalexander)
(Assignee)

Comment 13

3 years ago
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.
(Assignee)

Comment 14

3 years ago
Created attachment 8505842 [details] [diff] [review]
Part 0: cleanup. v1

r=me.
(Assignee)

Comment 15

3 years ago
Created attachment 8505844 [details] [diff] [review]
Part 1: avoid crash when Sync is partially configured. v2

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.
Attachment #8505844 - Flags: review?(nalexander)
(Assignee)

Updated

3 years ago
Whiteboard: [leave open]
(Assignee)

Comment 17

3 years ago
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?
Attachment #8505844 - Flags: review?(nalexander) → review+
(Assignee)

Comment 21

3 years ago
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?
Flags: needinfo?(nalexander)
(Assignee)

Comment 22

3 years ago
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.
(Assignee)

Comment 23

3 years ago
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.
(Assignee)

Comment 25

3 years ago
This'll definitely need verifying.
Flags: needinfo?(nalexander) → qe-verify?
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/ae1dfa192faf
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(Assignee)

Comment 27

3 years ago
I don't see why this is tracking 34; the feature only landed in 35.
tracking-fennec: 34+ → 35+
(Assignee)

Comment 28

3 years ago
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.
Attachment #8505844 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 29

3 years ago
(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.
Attachment #8505844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

3 years ago
status-firefox34: --- → unaffected
status-firefox35: --- → fixed
status-firefox36: --- → fixed
You need to log in before you can comment on or make changes to this bug.