761045 - Cannot add locally acquired apps on a machine to a user's browser ID account

Status: RESOLVED FIXED

(Web Apps Graveyard :: AppsInTheCloud, defect)

Description

[Jason Smith [:jsmith]]

Currently the AITC implementation does not allow a user to add locally acquired apps on a user's machine to a user's browser ID account. This is problematic, as before the AITC implementation is enabled, a user may have already acquired apps on the machine that may not be synced to the cloud. We need to provide some mechanism to allow a user to add locally acquired apps to the cloud for a specific browser ID account.

Comment 1

[Anant Narayanan [:anant]]

As discussed in the meeting today, this is a very low priority as the number of users who will already have apps locally installed before AITC is switched on by default will be very low.

Comment 2

[[:fabrice] Fabrice Desré]

What about people that won't enable aitc, install a bazillion of great apps, and then switch on aitc?

Comment 3

[Anant Narayanan [:anant]]

That's a good point. We could go two ways on this and say that if you turned off AITC any app activity during that period will not be synchronized, or we could write this code to port over all your existing apps.

I think it all depends on how we frame "turning off AITC". Is it more like "private browsing" mode or is it more like "work offline" mode?

Comment 4

[[:fabrice] Fabrice Desré]

It can be "I don't want to use that now - but maybe I'll change my mind later". And that should just work.

Comment 5

[Anant Narayanan [:anant]]

Sure, but you can't assume that's what the user means. Right now the only way to turn AITC off is by going to about:config anyway, so until that changes, I still think this is low priority. We have tons of other more important bugs to fix.

Comment 6

[[:fabrice] Fabrice Desré]

Wait, what? It is ON by default and with no UI to disable? Did that get a privacy review?

Comment 7

[Anant Narayanan [:anant]]

It is not ON by default yet. Hold your horses!

Comment 8

[Jason Smith [:jsmith]]

On a random note for a possible use case for this functionality:

Installing apps while not logged into marketplace or the myapps dashboard

Meaning areas such as:

- Installing apps off of appdir while not logged in
- Installing an app off the origin where the app is hosted

Comment 9

[Anant Narayanan [:anant]]

(In reply to Jason Smith [:jsmith] from comment #8)
> - Installing apps off of appdir while not logged in
> - Installing an app off the origin where the app is hosted

Those two cases will work just fine (apps will get pushed to the cloud next time the user logs in). This bug is to deal with only the case of whether or not apps installed when the AITC feature itself is turned off (or unavailable) will be pushed later.

Comment 10

[Anant Narayanan [:anant]]

This becomes important again, if the mozApps API is enabled before AITC is in a release version of Firefox.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Anant Narayanan [:anant] from comment #10)
> This becomes important again, if the mozApps API is enabled before AITC is
> in a release version of Firefox.

FYI - Something very strange happened in regards to this that warrants this bug in another use case (strangely enough, this wasn't happening when I first tested this feature):

1. Start firefox and login into dashboard with account #1 with apps
2. See the apps you've installed for that account
3. Close firefox
4. Start firefox and login into dashboard with account #2 with apps

Result - The local storage of apps from account #1 are gone.

Implications - The implementation appears to be now acting as a "per persona" account management of apps, disregarding anything local. That's weird, but that's definitely tied to this bug apparently.

The original case brought up in this bug note won't be able to happen if we don't backout anything from FF 15 - only Aurora or Nightly users might be affected that installed apps before the AITC implementation landed (i.e. leave the preffed off code in the firefox build, only enabled via a pref).

Comment 12

[Anant Narayanan [:anant]]

Attachment: Upload locally installed apps on first run

We check for "first-run" by looking at services.aitc.client.lastModified and add apps to the PUT queue if this is a fresh client.

There is the chance for the browser to restart before the PUT queue finishes and lastModified updated, in which case the same app may be added twice to the queue. Extra PUTs are harmless.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 633748 [details] [diff] [review]
Upload locally installed apps on first run

Review of attachment 633748 [details] [diff] [review]:
-----------------------------------------------------------------

I /think/ that this changes the behavior and now calls DOMApplicationRegistry as soon as the AITC service is started. Previously, we just installed some listeners and didn't talk to any other service.

If behavior did change, I'm curious what the performance implications of calling DOMApplicationRegistry on service start-up are. We *may* want to consider delaying service initialization on application start by a few seconds so we don't put more of a burden on the start-up procedure.

Also, where are the tests?

::: services/aitc/modules/manager.js
@@ +162,5 @@
> +  _initialSchedule: function _initialSchedule() {
> +    let self = this;
> +
> +    function startProcessQueue() {
> +      self._makeClient(function(err, client) {

Nit: name

@@ +198,5 @@
> +        let app = apps[appids[done]];
> +        let obj = {type: "install", app: app, retries: 0, lastTime: 0};
> +
> +        done += 1;
> +        self._pending.enqueue(obj, appQueued);

This would be much cleaner (and less expensive since the queue does file I/O) if you could enqueue multiple items with one API call. What do you think?

If we stick with the existing enqueue function, I'm a fan of iterating over the original set and calling enqueue linearly and then gating on all the callbacks to finish. I think this is clearer and safer than the recursive approach applied here.

@@ +232,5 @@
>        // resume after the PUTs finish (see processQueue)
>        this._processQueue();
>        return;
>      }
> +

Killed the whitespace \o/

Comment 14

[Anant Narayanan [:anant]]

(In reply to Gregory Szorc [:gps] from comment #13)
> I /think/ that this changes the behavior and now calls
> DOMApplicationRegistry as soon as the AITC service is started. Previously,
> we just installed some listeners and didn't talk to any other service.
> 
> If behavior did change, I'm curious what the performance implications of
> calling DOMApplicationRegistry on service start-up are. We *may* want to
> consider delaying service initialization on application start by a few
> seconds so we don't put more of a burden on the start-up procedure.

We already only initialize AITC after the "sessionstore-windows-restored" event which is pretty late after startup. Additionally, note that we only execute that block the very first time that the user starts using AITC (i.e. the last GET's timestamp is still 0). In light of these two, I think the performance implication of this is minimal.
 
> This would be much cleaner (and less expensive since the queue does file
> I/O) if you could enqueue multiple items with one API call. What do you
> think?
> 
> If we stick with the existing enqueue function, I'm a fan of iterating over
> the original set and calling enqueue linearly and then gating on all the
> callbacks to finish. I think this is clearer and safer than the recursive
> approach applied here.

Yeah, I think we may just have to change the enqueue function for this, but I don't want to block on it. We can't do this in parallel and collect the callbacks because in some cases the write lock kicks in causing one of the queues to fail. I think a linear approach (i.e. queuing an item only after the previous one finished) like this one is the safest.

Comment 15

[Gregory Szorc [:gps]]

(In reply to Anant Narayanan [:anant] from comment #14)
> (In reply to Gregory Szorc [:gps] from comment #13)
> > I /think/ that this changes the behavior and now calls
> > DOMApplicationRegistry as soon as the AITC service is started. Previously,
> > we just installed some listeners and didn't talk to any other service.
> > 
> > If behavior did change, I'm curious what the performance implications of
> > calling DOMApplicationRegistry on service start-up are. We *may* want to
> > consider delaying service initialization on application start by a few
> > seconds so we don't put more of a burden on the start-up procedure.
> 
> We already only initialize AITC after the "sessionstore-windows-restored"
> event which is pretty late after startup. Additionally, note that we only
> execute that block the very first time that the user starts using AITC (i.e.
> the last GET's timestamp is still 0). In light of these two, I think the
> performance implication of this is minimal.

Sounds good to me. Although, I would like someone with more experience to weigh in here since my knowledge here is far from complete.
 
> > This would be much cleaner (and less expensive since the queue does file
> > I/O) if you could enqueue multiple items with one API call. What do you
> > think?
> > 
> > If we stick with the existing enqueue function, I'm a fan of iterating over
> > the original set and calling enqueue linearly and then gating on all the
> > callbacks to finish. I think this is clearer and safer than the recursive
> > approach applied here.
> 
> Yeah, I think we may just have to change the enqueue function for this, but
> I don't want to block on it. We can't do this in parallel and collect the
> callbacks because in some cases the write lock kicks in causing one of the
> queues to fail. I think a linear approach (i.e. queuing an item only after
> the previous one finished) like this one is the safest.

I don't like it but I suppose it will be fine. As long as it is tested...

Comment 16

[Anant Narayanan [:anant]]

Attachment: Upload locally installed apps on first run

Added a couple of tests, cleaned up some more whitespaces, and corrected a typo.

Comment 17

[Anant Narayanan [:anant]]

Comment on attachment 637322 [details]
Upload locally installed apps on first run

Ignore that earlier patch, it was the wrong version.

Comment 18

[Anant Narayanan [:anant]]

Attachment: Upload locally installed apps on first run

There we go! Can we use git for m-c already... :)

Comment 19

[Anant Narayanan [:anant]]

Attachment: Upload locally installed apps on first run

And of course I forget to hg add, but hey, that happens in git all the time too.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 637326 [details] [diff] [review]
Upload locally installed apps on first run

Review of attachment 637326 [details] [diff] [review]:
-----------------------------------------------------------------

It works. But, we need some refactoring so we don't introduce possible oranges related to timing.

::: services/aitc/modules/manager.js
@@ +173,5 @@
> +
> +    if (Preferences.get("services.aitc.client.lastModified", "0") != "0") {
> +      if (this._pending.length) {
> +        startProcessQueue();
> +      }

Why not just check this._pending.length? i.e. if you don't want to upload if we've never synced before, please state why. I could easily see someone (like myself) thinking "we have items to upload - we should just upload them."

@@ +200,5 @@
> +
> +        done += 1;
> +        self._pending.enqueue(obj, appQueued);
> +      }
> +      appQueued();

I still think it is cleaner to iterate over enqueue() and invoke startProcessQueue() once enqueue's callback has been invoked total times. But, I'll let this slide.

::: services/aitc/tests/unit/test_manager.js
@@ +41,5 @@
> +  }
> +};
> +
> +function run_test() {
> +  run_next_test();

+initTestLogging().

@@ +53,5 @@
> +  // Create an instance of the manager and check if it put the app in the queue.
> +  let manager = new AitcManager(function() {
> +    // Here's where it gets ugly, the manager will add local apps to the queue
> +    // asynchronously, so we don't know for sure when to check if the app was
> +    // added. We'll wait for arbritrary 100ms (intermittent orange warning!).

Ouch. This can and will cause intermittent oranges. We can't have this.

Sometimes test code warrants API changes/features. This is one of those cases.

You can either change the callback passed into the constructor to fire when initial uploads have completed. Or, you can refactor the initial upload foo into a public function which takes an explicit, distinct callback. Whoever creates AitcManager instances (basically just the service) will need to invoke that manually. A little annoying, yes. But, it does result in more control and reliable tests.

Comment 21

[Anant Narayanan [:anant]]

Attachment: Upload locally installed apps on first run

Comment 22

[Anant Narayanan [:anant]]

Attachment: Upload locally installed apps on first run

Sorry, I forgot to qref a few more local changes in that previous patch. WTB qref reminder...

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 641975 [details] [diff] [review]
Upload locally installed apps on first run

Review of attachment 641975 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there.

::: services/aitc/modules/main.js
@@ +28,5 @@
>      Preferences.get("services.aitc.dashboard.url")
>    ).prePath;
>  
> +  let self = this;
> +  this._manager = new AitcManager(function() {

Nit: name

@@ +41,4 @@
>      let self = this;
>  
> +    // Do an initial upload.
> +    this._manager.initialSchedule(function(num) {

Nit: name

::: services/aitc/modules/manager.js
@@ +165,5 @@
> +  initialSchedule: function initialSchedule(cb) {
> +    let self = this;
> +
> +    function startProcessQueue(num) {
> +      cb(num);

Put this after the call to _makeClient?

@@ +166,5 @@
> +    let self = this;
> +
> +    function startProcessQueue(num) {
> +      cb(num);
> +      self._makeClient(function(err, client) {

Nit: name

@@ +179,5 @@
> +    // If we've already done a sync with AITC, it means we've already done
> +    // an initial upload. Resume processing the queue, if there are items in it.
> +    if (Preferences.get("services.aitc.client.lastModified", "0") != "0") {
> +      if (this._pending.length) {
> +        startProcessQueue();

startProcessQueue takes an argument but you don't pass one. This seems like it should raise a test failure. I guess you don't have test coverage here!

@@ +181,5 @@
> +    if (Preferences.get("services.aitc.client.lastModified", "0") != "0") {
> +      if (this._pending.length) {
> +        startProcessQueue();
> +      }
> +      cb(-1);

cb is also called in startProcessQueue.

::: services/aitc/tests/unit/test_manager.js
@@ +58,5 @@
> +
> +  function doInitialUpload() {
> +    manager.initialSchedule(function(num) {
> +      // Check that an initial upload was initiated.
> +      do_check_neq(num, -1);

This is redundant with below check.

Comment 24

[Anant Narayanan [:anant]]

Attachment: Upload locally installed apps on first run

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 642022 [details] [diff] [review]
Upload locally installed apps on first run

Review of attachment 642022 [details] [diff] [review]:
-----------------------------------------------------------------

Please also document why nextTick is used per verbal conversation.

::: services/aitc/modules/browserid.js
@@ +395,5 @@
> +    this._log = Log4Moz.repository.getLogger("Service.AITC.BrowserID.Sandbox");
> +    this._log.level = Log4Moz.Level[PREFS.get("log")];
> +    this._log.error("Could not create Sandbox " + e);
> +    cb(null);
> +  }

Could you add a comment saying why the try was added.

::: services/aitc/modules/main.js
@@ +30,5 @@
>  
> +  let self = this;
> +  this._manager = new AitcManager(function managerDone() {
> +    CommonUtils.nextTick(self._init, self);
> +  });

This /could/ be written as:

this._manager = new AitcManager(CommonUtils.nextTick.bind(this, this._init, this));

Comment 26

[Anant Narayanan [:anant]]

https://hg.mozilla.org/services/services-central/rev/1808d668c711

QA: Please verify that local apps installed on a fresh profile are uploaded the first time AITC is activated.

Comment 27

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/1808d668c711