florian / federated-learning-addon

11 stars 4 forks source link

The "Federated Learning" shield study is not removed after its expiration date #8

Open Softvision-RemusDranca opened 6 years ago

Softvision-RemusDranca commented 6 years ago

[Affected versions]:

[Affected Platforms]:

[Prerequisites]:

[Steps to reproduce]:

  1. Set the system date to two weeks after the current date.
  2. Open the browser with the profile from prerequisites and navigate to "about:addons">Extensions section.
  3. Observe the addons section.

[Expected result]:

[Actual result]:

[Notes]:

web

florian commented 6 years ago

@gregglind Is this a bug in the shield-studies-addon-utils repo or do I need to add additional code?

I tried using this code:

browser.study.onEndStudy.addListener(() => {
  optimizer.reset()
  browser.management.uninstallSelf()
})

Or, alternatively, just logging something instead of calling uninstall. The callback is not triggered if the time is over. I tested changing the system time as well as changing the shield.federated-learning_shield_mozilla_org.firstRunTimestamp pref to a very small value.

"Ending a study" in the FAQ makes it sound like the callback should be triggered here.

gregglind commented 6 years ago

Agreed it should be. Will debug tomorrow.

On Tue, Jul 10, 2018 at 3:55 PM Florian Hartmann notifications@github.com wrote:

@gregglind https://github.com/gregglind Is this a bug in the shield-studies-addon-utils https://github.com/mozilla/shield-studies-addon-utils/ repo or do I need to add additional code?

I tried using this code:

browser.study.onEndStudy.addListener(() => { optimizer.reset() browser.management.uninstallSelf() })

Or, alternatively, just logging something instead of calling uninstall. The callback is not triggered if the time is over. I tested changing the system time as well as changing the shield.federated-learning_shield_mozilla_org.firstRunTimestamp pref to a very small value.

"Ending a study" in the FAQ https://github.com/mozilla/shield-studies-addon-utils#faq makes it sound like the callback should be triggered here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/florian/federated-learning-addon/issues/8#issuecomment-403962642, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKAj6wEHl_H_FirBLfgwBcEmX8B0Cvyks5uFRSjgaJpZM4VJv_u .

gregglind commented 6 years ago

I am working on this. Automated tests work correctly. Trying to figure out what if anything is different about your procedure and usage.

gregglind commented 6 years ago

Note: I consider this NOT A BLOCKER, because we can hard kill studies.

Hard kill: Normandy forces an addon uninstall.

These are the risks:

mozilla/shield-studies-addon-utils#244 mozilla/shield-studies-addon-utils#246

motin commented 6 years ago

This bug was fixed in utils 5.0.2. I don't see what version is used in this add-on. Could you add utils as a dependency to package.json? See https://github.com/mozilla/shield-studies-addon-utils#installing-the-shield-studies-addon-utils-in-your-add-on - thanks :)

gregglind commented 6 years ago

(You have 5.0.1, trying to patch for you)

florian commented 6 years ago

@motin, @gregglind: Thanks, I wasn't aware of that!

The upgrade and https://github.com/florian/federated-learning-addon/pull/11 don't seem to fix this for me however. I tried triggering the expiry by changing the system date / changing the respective timestamp pref. If I then restart the browser, the pref is cleared to an empty value and studyInfo.delayInMinutes is null, so the alarm is not created.

motin commented 6 years ago

@florian Expiration is a bit tricky. I recommend comparing your implementation with the one in the template: https://github.com/mozilla/shield-studies-addon-template

The template also has functional tests that verifies that the expiration works as expected. You might want to lift in similar functional tests to your repo (like in https://github.com/mozilla/shield-cloudstorage/pull/48)

There are also some new docs coming regarding expiration, available here: https://github.com/mozilla/shield-studies-addon-utils/pull/248/files#diff-c320e75ed07805a5316c10c23ba7d297

If all fails, try retrofitting your study-logic into a fork of the template so that you have a better-tested base for the study add-on.