Last Comment Bug 697063 - Tabs.getSelectedTab returns null until Gecko starts and will cause crashes
: Tabs.getSelectedTab returns null until Gecko starts and will cause crashes
Status: RESOLVED FIXED
[native-crash]
: crash
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: Sriram Ramasubramanian [:sriram]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-25 05:53 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2012-01-09 15:34 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Patch (1.30 KB, patch)
2011-10-25 10:33 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review-
Details | Diff | Splinter Review
Patch (4.53 KB, patch)
2011-10-25 11:04 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review-
Details | Diff | Splinter Review
Patch (4.44 KB, patch)
2011-10-25 12:22 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 05:53:14 PDT
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".
Comment 1 Sriram Ramasubramanian [:sriram] 2011-10-25 10:33:40 PDT
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.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 10:39:31 PDT
Comment on attachment 569417 [details] [diff] [review]
Patch

What about all the other failure modes? Like "Share" and "Reload"
Comment 3 Sriram Ramasubramanian [:sriram] 2011-10-25 11:04:24 PDT
Created attachment 569425 [details] [diff] [review]
Patch

Reload, Bookmarks and Share have been take care of in this patch.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 11:30:46 PDT
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
Comment 5 Sriram Ramasubramanian [:sriram] 2011-10-25 11:33:56 PDT
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.
Comment 6 Sriram Ramasubramanian [:sriram] 2011-10-25 12:22:49 PDT
Created attachment 569463 [details] [diff] [review]
Patch

Refactored the code as requested.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 13:14:27 PDT
pushed https://hg.mozilla.org/projects/birch/rev/4c926c4ac01b

Note You need to log in before you can comment on or make changes to this bug.