1154473 - Pre-resolve queued URLs

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Overlays, defect)

Description

[Richard Newman [:rnewman]]

All those queued t.co links could usefully be followed immediately after queuing, so I don't have to wait when I finally unqueue them. We can attempt this for whitelisted redirectors that we know will give HTTP (not JS) redirects.

Comment 1

[Richard Newman [:rnewman]]

See also Bug 985791.

Comment 2

[Richard Newman [:rnewman]]

Any update, Sebastian?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1154473_resolver_v1.patch

(In reply to Richard Newman [:rnewman] from comment #2)
> Any update, Sebastian?

Yes! I attached my current version of the patch. I have been running into a couple of problems that I haven't solved yet:

* My first intention was to move this functionality to a class in the util package so that it could be used by other components in the future (because of bug 985791). However I struggled with the build system because right now the class has a dependency on GeckoAppShell to get the user agent and utils seem to be one of the first things being compiled (before GeckoAppShell). Maybe right now it would be more straight-forward to move it to the tabqueue package and relocate it once actually needed somewhere else.

* Since the functionality is independent from the rest of the code, I wanted to write a unit test for it. There's no framework/infrastructure right now for running plain unit tests, right? I tried to build something with UITest (just for testing the code on my machine) but I didn't really grasp how the Robocop tests are build (they are not using moz.build files, right?): The class can't be found while building the test.

* The current implementation resolves the URL before adding it to the queue and showing the notification. In a situation with a bad network connection this will lead to a delayed queuing of the URL (~ connection timeout + read timeout + x). I would like to avoid that. Imagine you queue something, then switch to the browser and the URL is not queued just yet. However queuing the original URL, then resolving the URL and replacing the URL in the queue might introduce a new set of concurrency issues. Maybe just using very strict timeouts can prevent the problem as well (-> URLs are only resolved if the connection is good and fast). Right now the timeout values are just guestimates and might need tweaking anyways.

* Right now I'm using GeckoAppShell to get the default User Agent but the method getDefaultUAString() looks like there's a way to get a non-default user agent (some overriden US by an extension maybe?). In bug 985791 there existed some getUAForHost() method but this one seems to be gone.

* t.co is a special case: Whenever I make a request using the Firefox UA, I get a HTTP 200 response and a JavaScript redirect. With any other random nonsense UA I'll get a HTTP 301 response and a Location header. I'm not sure if we want to fake the UA for the sake of resolving these urls.

Comment 4

[Richard Newman [:rnewman]]

(In reply to :Sebastian Kaspari from comment #3)

> * Since the functionality is independent from the rest of the code, I wanted
> to write a unit test for it.

./mach build build/mobile/robocop, I think.

Then ./mach robocop testWhatever.

Take a look at the non-UI Robocop tests: testDBUtils.java, for example.

You will need to annotate your classes with @RobocopTarget so that ProGuard doesn't throw them away. (Grep for that string.)


> …Imagine you queue something, then
> switch to the browser and the URL is not queued just yet.…

How about maintaining a lookup table alongside the queue? Queue up the tabs to be opened, and spawn a lookup. Write the successful result of the lookup into the lookup table.

When the tab comes to be opened, look in the lookup table to see if we already know a better URL to start at.

> 
> * Right now I'm using GeckoAppShell to get the default User Agent but the
> method getDefaultUAString() looks like there's a way to get a non-default
> user agent (some overriden US by an extension maybe?).

I want GAS's management of the UA string to die.

Note that it ends up here:

    public String getDefaultUAString() {
        return HardwareUtils.isTablet() ? AppConstants.USER_AGENT_FENNEC_TABLET :
                                          AppConstants.USER_AGENT_FENNEC_MOBILE;
    }

See what SuggestClient does, which can best be described as "ah, fuck it":


    // This should go through GeckoInterface to get the UA, but the search activity
    // doesn't use a GeckoView yet. Until it does, get the UA directly.
    private static final String USER_AGENT = HardwareUtils.isTablet() ?
        AppConstants.USER_AGENT_FENNEC_TABLET : AppConstants.USER_AGENT_FENNEC_MOBILE;

Sever that dependency on GAS!


> In bug 985791 there
> existed some getUAForHost() method but this one seems to be gone.

Yeah, it got backed out. Follow the breadcrumbs to Bug 713503.


> * t.co is a special case: Whenever I make a request using the Firefox UA, I
> get a HTTP 200 response and a JavaScript redirect. With any other random
> nonsense UA I'll get a HTTP 301 response and a Location header. I'm not sure
> if we want to fake the UA for the sake of resolving these urls.

When we fetch inside Gecko we use site-specific user agents for this kind of thing. In Java you have to follow an approach like Bug 713503 to try to get a network redirect.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you for the hints, Richard! The lookup table is a good idea, I'll try implementing that! :)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1154473_resolver_v2.patch

I added a new patch that resolves the URLs on a separate executor and stores them in a file along with the queue.

* It's working great but I somehow feel it's adding too much complexity to the code. What's your opinion on resolving the URLs before queuing them but using very strict timeouts? Resolving URLs is convenient but not necessary - I assume the long-term solution will be to load the whole tab in the background?

* I attached the test to the patch too. Right now it's testing against the live servers of the whitelisted redirectors. This is probably quite unfriendly. A first try push[1] seems to indicate that during testing there is no internet connection anyways. With a mocking framework I could just test the behavior of the class. Do you have an idea how I could still test this without leaving the scope of this bug? :)

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4309e5d17f96

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Richard, do you have any feedback for me? :) (See comment & patch above).

Comment 8

[Richard Newman [:rnewman]]

(In reply to :Sebastian Kaspari from comment #6)

> * It's working great but I somehow feel it's adding too much complexity to
> the code.

From a quick skim it looks like most of the added complexity is around persisting the resolved URLs.

There are a few ways we can make that better:

1. Two-stage queuing. Queue 1 is raw URLs. Queue 2 is resolved. Items move from one to the next. When Fennec is launched, it pulls from both queues. This might not be a complexity win.

2. (a) In-place replacement. If we successfully resolve, re-queue the URL, removing the original one at the same time. This runs the risk of resolving to the wrong URL (e.g., login required, UA sniffing). It means two file writes, but hey, that's the cost of reliability.

2. (b) In-place replacement with preserved original URL; presumably we could then open a tab with the original URL already in the back stack, just in case (a follow-up bug). This likely involves an extension to the tab queue format.


> What's your opinion on resolving the URLs before queuing them but
> using very strict timeouts? Resolving URLs is convenient but not necessary -

Losing queued URLs has a really big impact on perceived reliability, so I'm strongly in favor of solutions that don't increase the chance of failure. Probably there is no timeout value for resolve-then-write that will both reliably resolve t.co on an EDGE network and also is robust against data loss.

If we were to take this approach, I'd rather do (1), above.


> I assume the long-term solution will be to load the whole tab in the
> background?

Yes, but that's been a long-term solution for more than a year now, so I wouldn't block a good solution here on that.


> Do you have an idea how I could still test
> this without leaving the scope of this bug? :)

Correct, there is no network access allowed during testing. There are two 'integration' ways to test this:

* Mock out the redirect lookup, and verify that when the tab eventually loads, it hits the right (local, mochi.test) URL.

* Add a local redirector to the whitelist, and make the added URL match against it. This is as close to live testing as you can get.

In both cases you'll need to start a test HTTP server. There are probably examples of that in the tree.

You can also just unit test the redirect lookup as best you can, and rely on QA.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #8)
> Losing queued URLs has a really big impact on perceived reliability, so I'm
> strongly in favor of solutions that don't increase the chance of failure.
> Probably there is no timeout value for resolve-then-write that will both
> reliably resolve t.co on an EDGE network and also is robust against data
> loss.

I totally agree here. My intention was to queue the unresolved URL whenever resolving fails. So in the worst case the URL has to be resolved when the user enters the browser - but it is never lost. The strict timeout was just to avoid that queuing the tab is heavily delayed.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Unassigning for now.

Some additional notes:
* We might get closer to background loading soon for supporting custom tabs - bug 1208655
* Another option for solving the problem of t.co JS redirects could be using the ua override mechanism, see bug 1178760

Comment 12

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.