Closed Bug 726080 Opened 9 years ago Closed 9 years ago

Don't set searches as tab URLs

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13
Tracking Status
blocking-fennec1.0 --- +
fennec + ---

People

(Reporter: bnicholson, Assigned: bnicholson)

Details

Attachments

(1 file)

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.
tracking-fennec: --- → ?
tracking-fennec: ? → +
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
Attached patch patchSplinter Review
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).
Attachment #602447 - Flags: review?(mark.finkle)
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
Attachment #602447 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/8b8c280f4df0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.