The default bug view has changed. See this FAQ.

Tabs.getSelectedTab returns null until Gecko starts and will cause crashes

RESOLVED FIXED

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mfinkle, Assigned: sriram)

Tracking

({crash})

unspecified
ARM
Android
crash
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

(Whiteboard: [native-crash])

Attachments

(1 attachment, 2 obsolete attachments)

We have a lot of code that gets the selected tab and preformsa an action:

Tab tab = Tabs.getInstance().getSelectedTab();
// do something to tab

Currently, there are no "tabs" until gecko starts, so trying to perform one of those actions too early during startup will result in a crash. We could add null checks to all the code spots, or we could create a dummy tab right away.

I really don't know which is better. I suppose a dummy tab _could_ make some actions seem to work, like "Share", but others would be useless, like "Reload".
(Reporter)

Updated

6 years ago
OS: Linux → Android
Priority: -- → P1
Hardware: x86 → ARM
(Assignee)

Comment 1

6 years ago
Created attachment 569417 [details] [diff] [review]
Patch

This patch makes sure that the awesomebar is shown without crashing when gecko is loading.
When gecko is loading, the url is not shown though.
Assignee: nobody → sriram
Attachment #569417 - Flags: review?(mark.finkle)
Comment on attachment 569417 [details] [diff] [review]
Patch

What about all the other failure modes? Like "Share" and "Reload"
Attachment #569417 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 569425 [details] [diff] [review]
Patch

Reload, Bookmarks and Share have been take care of in this patch.
Attachment #569417 - Attachment is obsolete: true
Attachment #569425 - Flags: review?(mark.finkle)
Comment on attachment 569425 [details] [diff] [review]
Patch


>            case R.id.bookmarks:
>                Intent intent = new Intent(this, GeckoBookmarks.class);
>                tab = Tabs.getInstance().getSelectedTab();
>+               if (tab == null) {
>+                   startActivity(intent);
>+                   return true;
>+               }
>+
>                he = tab.getLastHistoryEntry();
>                if (he != null) {
>                    intent.setData(android.net.Uri.parse(he.mUri));
>                    intent.putExtra("title", he.mTitle);
>                    startActivity(intent);
>                }
>                return true;

We need to convert this to "Add/Remove Bookmark" toggle. Showing the list is now part of Awesombar. In a new bug I guess.

>         if (aType != AwesomeBar.Type.ADD) {
>             // if we're not adding a new tab, show the old url
>             Tab tab = Tabs.getInstance().getSelectedTab();
>+            if (tab == null) {
>+                startActivityForResult(intent, AWESOMEBAR_REQUEST);
>+                return true;
>+            }
>+
>             if (!tab.getHistory().empty()) {
>                 intent.putExtra(AwesomeBar.CURRENT_URL_KEY, tab.getHistory().peek().mUri);
>             }
>         }
>         startActivityForResult(intent, AWESOMEBAR_REQUEST);
>         return true;

I don't like having two "startActivityForResult". Wouldn't this work?

if (tab != null && !tab.getHistory().empty()) {
...
}


>diff --git a/embedding/android/GeckoBookmarks.java b/embedding/android/GeckoBookmarks.java

fine for now, but we should probably remove/refactor GeckoBookmarks.java since the list is now in Awesombar

r- for the startActivityForResult change
Attachment #569425 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 5

6 years ago
startActitivyForResult is needed so that whatever user types in the AwesomeBar gets loaded. startActivity does not get the value from AwesomeBar. The whole purpose of faking the user to type the url when gecko is loading would fail if we do that.
(Assignee)

Comment 6

6 years ago
Created attachment 569463 [details] [diff] [review]
Patch

Refactored the code as requested.
Attachment #569425 - Attachment is obsolete: true
Attachment #569463 - Flags: review?(mark.finkle)
(Reporter)

Updated

6 years ago
Attachment #569463 - Flags: review?(mark.finkle) → review+
pushed https://hg.mozilla.org/projects/birch/rev/4c926c4ac01b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Keywords: crash
Whiteboard: [native-crash]
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.