Closed Bug 697063 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: mfinkle, Assigned: sriram)

Details

(Keywords: crash, Whiteboard: [native-crash])

Attachments

(1 file, 2 obsolete files)

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".
OS: Linux → Android
Priority: -- → P1
Hardware: x86 → ARM
Attached patch Patch (obsolete) — Splinter Review
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-
Attached patch Patch (obsolete) — Splinter Review
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-
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.
Attached patch PatchSplinter Review
Refactored the code as requested.
Attachment #569425 - Attachment is obsolete: true
Attachment #569463 - Flags: review?(mark.finkle)
Attachment #569463 - Flags: review?(mark.finkle) → review+
pushed https://hg.mozilla.org/projects/birch/rev/4c926c4ac01b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Keywords: crash
Whiteboard: [native-crash]
tracking-fennec: --- → 11+
You need to log in before you can comment on or make changes to this bug.