1339976 - Send Tab Extension incorrectly updates application in the client record

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect, P1)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

+++ This bug was initially created as a clone of Bug #1339861 +++

As Alex reports in that bug:

1) There is a device called "Send Tab" which is actually Firefox release for iOS. So the name here is no good.

And as Kit points out:

(In reply to Kit Cambridge [:kitcambridge] from bug 1339861 comment #4)
> (In reply to Ryan Kelly [:rfkelly] from bug 1339861 comment #3)
> > I remember hearing about this before, it may be an existing bug, or even
> > fixed in the pending release.  Grisha, does this ring any bells?
> 
> There was a mailing list thread about this a couple of weeks ago. Quoting
> rnewman:
> 
> > When we upload a client record, we do so by asking the app bundle what
> > the app's name is — "Firefox" or "Nightly" — and what the device name
> > is — "Ryan's iPhone" — and then pushing those into a template string.
> > That's the client name: "Firefox on Ryan's iPhone". We save that in
> > prefs for next time.
> > 
> > What I think happened in your case is that the first piece of code
> > to sync on your device was the Send Tab extension. Normally a user
> > sets up Sync in the main Firefox app, and it syncs, so you'd see
> > "Firefox" there. It seems that in an app extension that the extension
> > has its own name, and its own bundle, so the app name we got was
> > "Send Tab".

That thread was on the fxa-staff and sync-staff mailing lists. The thread also calls out James Hugman who may have more insights, so needinfo him.

Comment 1

[James Hugman [:jhugman] [@jhugman]]

This looks like app bundle identifier in the Send Tab extension not being propagated.

Comment 2

[Stephan Leroux [:sleroux]]

James does it make sense to do this for 7.0?

Comment 3

[Richard Newman [:rnewman]]

See Comment 0, James :)

One way this might happen:

- You set up FxA with an unverified account.
- You background the app.
- You verify your FxA on desktop.
- You send a tab from an app on iOS without first opening Firefox.

Comment 5

[Richard Newman [:rnewman]]

This is not a duplicate of Bug 1306315.

That bug will rename "Send Tab" to something like "Send to Firefox".

This bug is about pulling out the same string in both the main app and in the extension, so that regardless of which one syncs first we get a coherent device name.

Bug 1306315 alone would mean we'd just get a device named "Richard's Send to Firefox on iPhone 6" instead.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The problem here is that when we upload a new Client Record from an App Extension, we take the wrong bundle identifier and app name.

This is similar to bug 1378878 where we fixed the appPackage from being updated. We also need to make sure we set the appName correctly.

Moving this bug to 8.0

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The problem here is that when we upload a new Client Record from an App Extension, we take the wrong bundle identifier and app name.

This is similar to bug 1378878 where we fixed the appPackage from being updated. We also need to make sure we set the appName correctly.

Moving this bug to 8.0

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR https://github.com/mozilla-mobile/firefox-ios/pull/2897

The problem here is that the `ClientSynchronizer` uses `DeviceInfo.appName()` to set the `"application"` field in the client record, which is the name of the current process and not of the actual top level application. So when you send a tab using the Send Tab *App Extension*, this value is set to `Send Tab`. (Not localized).

This patch fixes that by using `AppInfo.displayName`, which internally calls the new `AppInfo.applicationBundle` to determine the actual top level `Bundle` from which we can grab the application info.

I have made `Appinfo.displayName` return an optional value. Since there are a number of things that can go wrong. For consistency I have also made the `appVersion`, `buildNumber` and `majorAppVersion` variables in `AppInfo` optionals. In places where we use those values, I either check with an `if let` if the value could be obtained or I set `0` or `0.0` as the default value in case it cannot.

(I don't expect it to fail - but I have a bad feeling about just doing explicit unwraps)

Comment 9

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I updated the patch:

This patch previously allowed lookups in the Bundle to fail. But after thinking more about that, I think it is a valid case to crash with a fatal error if the app name, version or build number cannot be found. Those values *must* be present in the bundle. iTunes Connect will not let you publish an app without them.

Also, if they fail to load during runtime, then something really really bad happened and I don't think there is a point in continuing with some default value instead.

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Landed on master, uplifted to v8.x

Comment 11

[Justin D'Arcangelo [:justindarc]]

(In reply to Stefan Arentz [:st3fan] from comment #9)
> I updated the patch:
> 
> This patch previously allowed lookups in the Bundle to fail. But after
> thinking more about that, I think it is a valid case to crash with a fatal
> error if the app name, version or build number cannot be found. Those values
> *must* be present in the bundle. iTunes Connect will not let you publish an
> app without them.
> 
> Also, if they fail to load during runtime, then something really really bad
> happened and I don't think there is a point in continuing with some default
> value instead.

Ok. My only real concern was looking up these values from within an app extension. If we believe our code for retrieving values from an app extension is robust then this should be acceptable.