726080 - Don't set searches as tab URLs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Before a search is resolved to a URL, the tab's URL is set to the search term. That is, when searching for "foo", the tab's URL is set to "foo" before changing to "http://www.google.com?q=foo". This can break things that expect a URL, such as the getScheme() NPE in bug 720509. We should only set actual URLs.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: patch

A problem here is that we use LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP to resolve URLs from queries, which means contacting an external server. We won't have a valid URI for the tab until we get a response, but we also don't want to wait for a response before we add the tab. So between the time we add the tab and the time when the URI is updated, the tab's URL will be set to some non-URI value. Currently, this value is the query itself (e.g., the user searches "foo", so the tab's URL is temporarily set to "foo").

This patch makes the tab's URL set to null until we have a valid URI. Although not ideal, it at least means tabs have a single, standard value before they are set to a URI. This means we a can easily check whether tab.getURL() is null to ensure the URI is valid (we don't want to be able to add a bookmark or share "foo" before it has been resolved).

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 602447 [details] [diff] [review]
patch

>diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java

>     public Tab addTab(JSONObject params) throws JSONException {

>         String url = params.getString("uri");
>+        // null strings return "null" (http://code.google.com/p/android/issues/detail?id=13830)
>+        if (url.equals("null"))

nit: add a blank line above the comment

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    // only set tab uri if uri is valid
>+    let uri = null;
>+    try {
>+      uri = Services.io.newURI(aURL, null, null).spec;
>+    } catch (e) { /* optional */ }

aURL is not really optional, but might not be a real URI so just remove the "/* optional */" comment

Comment 3

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8b8c280f4df0

Comment 4

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/8b8c280f4df0