879192 - Add preference/setting for disabling app update notifications

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Zac C (:zac)]

We've been unsuccessful in disabling app updates on Gaia.

We want to disable it because the app update slides down and interferes with the application.

What we have tried
Setting settings:
app.update.interval to 0
app.update.interval to monthly

pushing to user.js and rebooting:
pref("app.update.enabled", false);
pref("app.update.auto", false);

MAuro suggested:
app.update.lastUpdateTime.background-update-timer -> to a future epoch value
idle.lastDailyNotification to 0

neither worked.

Dave and I have both experimented with setting the ftu urls to invalid/null values with no luck either.

The app update just keeps coming back like the Terminator!

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Etienne, any idea how we can proceed here? Between tests we remove the profile and any indexedDB files. This means each test runs in a clean state, however it also means we get the app update notifications every time.

Comment 2

[Etienne Segonzac (:etienne)]

(In reply to Zac C (:zac) from comment #0)
> The app update just keeps coming back like the Terminator!

It's a resilient system :)

Quick question, now that HERE is out of the build, which app update are you seeing?

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

I currently see:

2 updates available
Marketplace, 27.61 kB
PackStubTest, 5.83 kB

Comment 4

[Etienne Segonzac (:etienne)]

Don't know the details, but can we use |cache_sync.py| just before flashing to have already up to date external apps?

Or maybe flash without external apps?

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Where would we find cache_sync.py? Also, how can we flash without external apps? I'm not sure this will work for us because we have tests for the Marketplace app.

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Dave Hunt (:davehunt) [away 07/29 - 08/02] from comment #5)
> Where would we find cache_sync.py? 

In the gaia repo, external_apps directory.

> I'm not sure this will work for us because we have tests for the
> Marketplace app.

The market place is a hosted app, seems rather fragile...

A diametrically opposed approach would be to force-check updates at start and wait for the banner to go away. It's not pretty but I guess we're already waiting a bit in some tests if those are dependent from the network.

Comment 7

[Zac C (:zac)]

Thing is even if you force check and there are 0 updates then there will be no banner! It's that inconsistency and also the unknown timing of when the banner will pop up that is hurting us.

The cache_sync sounds promising but we'd have to get releng onboard with that.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(In reply to Etienne Segonzac (:etienne) from comment #6)
> (In reply to Dave Hunt (:davehunt) [away 07/29 - 08/02] from comment #5)
> > Where would we find cache_sync.py? 
> 
> In the gaia repo, external_apps directory.

In many cases we're just using the nightly builds and not checking out the gaia repository, so this will not be available to our test jobs. I suppose we could download https://raw.github.com/mozilla-b2g/gaia/master/external-apps/cache_sync.py in the job and run it. I can try this once I have a network connection for my device (unless :zac gets to it first)

> > I'm not sure this will work for us because we have tests for the
> > Marketplace app.
> 
> The market place is a hosted app, seems rather fragile...

These are no longer run as part of the Gaia functional UI tests. Can you explain why these would be fragile and how we might improve that for the Marketplace team?

> A diametrically opposed approach would be to force-check updates at start
> and wait for the banner to go away. It's not pretty but I guess we're
> already waiting a bit in some tests if those are dependent from the network.

As Zac says, if there are no updated then we're waiting for a banner which will never appear. It's also a delay to the start of every test, which is dependent on the network connection (not all tests connect the device to cell data/wifi)

Is there no preference available to simply disable these updates? Even if this was just introduced for testing purposes it would be incredibly valuable. I learned this week that this is also a blocker for the Eideticker tests.

Comment 9

[Jason Smith [:jsmith]]

Some comments here:

* cache_sync.py isn't going to help you here. The two apps in question are packaged apps, not hosted apps with appcache.
* The fragility comment is likely due to the fact that it's dependent on the network, where as other core Gaia apps might not be. There's also the fact that the app in the repository will usually not be the most up to date marketplace app - only the app from marketplace would be. That means we'll get app updates found for marketplace commonly.
* app.update.enabled to false won't prevent all 3rd party app update checks, as we found out in bug 897287. For example, app update checks will happen automatically when the FTU is completed.
* In reference to the external apps discussion - What Etienne is describing here is that app updates are going to be checked for any preinstalled app, but if you did not have any preinstalled apps in the build, then you won't have app update checks done. So a simple way to solve this problem without a preference is to run automation against a build with no preinstalled apps by default. If there's a special need for a preinstalled app, then include it in the test as needed with a customization.

Comment 10

[Zac C (:zac)]

Although we can install marketplace on the fly it would be counter to the efforts of Kumar and Marketplace team to get marketplace better integrated with prefs set during the build process.
There's also a technical difference between installing the app with a manifest file and installing it during the build and the latter is preferred for Marketplace.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Zac C (:zac) from comment #10)
> Although we can install marketplace on the fly it would be counter to the
> efforts of Kumar and Marketplace team to get marketplace better integrated
> with prefs set during the build process.
> There's also a technical difference between installing the app with a
> manifest file and installing it during the build and the latter is preferred
> for Marketplace.

That's true, although that does introduce the fragility problem indicated above that the marketplace included in the build process might not be the latest app available. Suppressing app updates via a preference will also include that fragility problem as well.

One idea I have for solving this problem would be to use the navigator.mozApps.mgmt API to get the marketplace app, check for updates, and download and apply the update during a global setup process for gaia ui tests. That would guarantee that the preinstalled apps you are using in your tests have the latest bits and as a result, should prevent you from getting app update prompts.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(In reply to Jason Smith [:jsmith] from comment #11)
> One idea I have for solving this problem would be to use the
> navigator.mozApps.mgmt API to get the marketplace app, check for updates,
> and download and apply the update during a global setup process for gaia ui
> tests. That would guarantee that the preinstalled apps you are using in your
> tests have the latest bits and as a result, should prevent you from getting
> app update prompts.

If we can avoid the notifications this way then it would seem a reasonable workaround, but I'd still prefer to see a way to disable the updates altogether. Ideally we don't want our automation making excessive connections to external resources.

Comment 13

[Etienne Segonzac (:etienne)]

We can replicate the app.update.enabled pref in a mozSetting and have it bypass the update prompt entirely. As long as it's well tested I'm sure everybody should be okay with this change.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Thanks Etienne, I've changed the summary for this bug to apply to the proposed fix. I've assigned it to you for lack of knowing who might work on this, but please reassign as necessary.

Comment 15

[William Lachance (:wlach)]

Ping. Is this still going to happen? As mentioned before, this is basically a blocker for getting reliable eideticker results in automation, as the flashing update notification interferes with getting reliable camera-based measurements.

Comment 16

[Etienne Segonzac (:etienne)]

(In reply to William Lachance (:wlach) from comment #15)
> Ping. Is this still going to happen? As mentioned before, this is basically
> a blocker for getting reliable eideticker results in automation, as the
> flashing update notification interferes with getting reliable camera-based
> measurements.

My bad.
Was next on my list, then the work week happened.

On it.

Comment 17

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Here is a patch honoring a new |gaia.system.disableUpdates| setting which disable the update notifications altogether.

Zac can you confirm that it does the trick?

Comment 18

[Zac C (:zac)]

Etienne I'm not in a position to self build right now.

Can anybody else test it for me?

Comment 19

[Etienne Segonzac (:etienne)]

Maybe Dave?

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 806684 [details] [diff] [review]
Patch proposal

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

All this feels like a hack.

Can't we disable the update subsystem instead ?

Comment 21

[Julien Wajsberg [:julienw]]

Fabrice, do you think this could be a good idea to disable the app updates mechanism using the  app.update.enabled pref, or another pref or settings ?

Comment 22

[Julien Wajsberg [:julienw]]

I know eng builds don't have updates at all. Could be another idea to make your tests work ?

Comment 23

[Zac C (:zac)]

Engineering builds do get updates; see comments #3 - #5

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Fabrice, do you think this could be a good idea to disable the app updates
> mechanism using the  app.update.enabled pref, or another pref or settings ?

Unfortunately I added a secondary timer to make sure that we are doing daily app update checks even if the app.update.enabled pref is disabled or if the delay is set to something different than 1 day (we need that to track the ADU pings).

What happens if you 1) turn off app.update.enabled pref and 2) don't send the force-update-check from gaia?

I'd really like to not add another kill-switch on the gecko side, since they tend to be misused.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

> What happens if you 1) turn off app.update.enabled pref and 2) don't send the force-update-check from gaia?

I understood that the force-update-check is happening when we kill the FTU app, so I don't see a way to prevent that as it's always running after a flash. Unless we change the eng builds to not start with the FTU, which was proposed in bug 876723.

Comment 26

[[:fabrice] Fabrice Desré]

ok, so if the tests have chrome privileges, what you should do is override the @mozilla.org/updates/update-checker;1 component and implement a no-op checkForUpdates() method in your mock.

Comment 27

[Etienne Segonzac (:etienne)]

Zac, would Fabrice's solution work?

Comment 28

[Julien Wajsberg [:julienw]]

Fabrice, I think we'll need more information on how to do this... we're not gecko wizards ;) A pointer to MDN could be enough.

Comment 29

[Zac C (:zac)]

+1 to that Julien, Bebe spent a bit of time on it yesterday but needs a bit of help too!

Comment 30

[[:fabrice] Fabrice Desré]

There are two solutions:
- register the component at build time in a .manifest, if there is already something like that test harness.
- register the component at run time. See for example what we do in https://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentHandler.js#84

Comment 31

[Etienne Segonzac (:etienne)]

What's the status here?
Can we confirm the solution from Comment 26?

Comment 32

[Etienne Segonzac (:etienne)]

missing ni

Comment 33

[Zac C (:zac)]

Sorry, none of us on the QA team have the technical skill to do this.

We need help from either a dev or the automation team.

Comment 34

[Etienne Segonzac (:etienne)]

Hey, we're trying to see if we can do the solution proposed on Comment 26, any idea?

Comment 35

[Jonathan Griffin (:jgriffin)]

(In reply to Etienne Segonzac (:etienne) from comment #34)
> Hey, we're trying to see if we can do the solution proposed on Comment 26,
> any idea?

I can't find much information about overriding XPCOM components; does the approach outlined here work?  http://stackoverflow.com/questions/5559849/how-to-overwrite-built-in-xpcom-component-in-firefox-addon

Comment 36

[[:fabrice] Fabrice Desré]

Yes, that's the idea!

Comment 37

[Jonathan Griffin (:jgriffin)]

(In reply to Fabrice Desré [:fabrice] from comment #26)
> ok, so if the tests have chrome privileges, what you should do is override
> the @mozilla.org/updates/update-checker;1 component and implement a no-op
> checkForUpdates() method in your mock.

Wouldn't it be easier to add a pref to disable checkForUpdates()?  This would have the advantage of not requiring harness-specific code; it could be done anywhere.

Comment 38

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

I just tried working around this by removing the apps with updated from the device. Unfortunately, Marketplace cannot be removed and therefore still show the update notification.

Comment 39

[[:fabrice] Fabrice Desré]

(In reply to Jonathan Griffin (:jgriffin) from comment #37)
> (In reply to Fabrice Desré [:fabrice] from comment #26)
> > ok, so if the tests have chrome privileges, what you should do is override
> > the @mozilla.org/updates/update-checker;1 component and implement a no-op
> > checkForUpdates() method in your mock.
> 
> Wouldn't it be easier to add a pref to disable checkForUpdates()?  This
> would have the advantage of not requiring harness-specific code; it could be
> done anywhere.

It would be easier for sure, but I really fear that it could be misused in production builds (fool me once, etc.).

Comment 40

[Julien Wajsberg [:julienw]]

Comment on attachment 806684 [details] [diff] [review]
Patch proposal

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

All this feels like a hack.

I'd really prefer to have a solution that would not use this but well...

The code looks ok, mostly nits, the comments are mostly in the test part.

As a side note, I think that, to be complete, if the update notification was disabled and is reenabled, we should call displayNotificationAndToaster again. I don't really mind if we do it now or later, as we currently have no mean to enable/disable this setting while running anyway, but this looks simple enough to do it now :)

::: apps/system/js/update_manager.js
@@ +97,5 @@
>  
>      SettingsListener.observe('gaia.system.checkForUpdates', false,
>                               this.checkForUpdates.bind(this));
>  
> +    SettingsListener.observe('gaia.system.disableUpdates', false,

Should we call it 'gaia.system.disableUpdateNotifications' instead ?

@@ +100,5 @@
>  
> +    SettingsListener.observe('gaia.system.disableUpdates', false,
> +    (function(value) {
> +      this._notificationsDisabled = value;
> +    }).bind(this));

nit: please indent this properly.

::: apps/system/test/unit/update_manager_test.js
@@ +787,5 @@
> +      });
> +
> +      suite('when the setting is not set', function() {
> +        setup(function() {
> +          MockSettingsListener.mCallbacks['gaia.system.disableUpdates']();

when the setting is not set, it returns the default value, so you should call it with `MockSettingsListener.mDefaultValue` as a parameter.

@@ +791,5 @@
> +          MockSettingsListener.mCallbacks['gaia.system.disableUpdates']();
> +        });
> +
> +        test('should let the toaster come up', function() {
> +          UpdateManager.displayNotificationAndToaster();

shouldn't you use sinon's fake timers in the setup, and call `clock.tick` instead ?

@@ +804,5 @@
> +          MockSettingsListener.mCallbacks['gaia.system.disableUpdates'](true);
> +        });
> +
> +        test('should prevent the toaster from coming up', function() {
> +          UpdateManager.displayNotificationAndToaster();

same here

@@ +1290,2 @@
>          assert.equal(UpdateManager.checkForUpdates.name,
> +                     observeSpy.firstCall.args[2].name);

I know this is old code, but you're modifying it.

Why don't we simply check that the arg _is_ UpdateManager.checkForUpdates ? Why do we assert using their names ?

I'd use:

  assert.isTrue(observeSpy.calledWith(
    'gaia.system.checkForUpdates',
    false,
    UpdateManager.checkForUpdates
  ));

And I'm not sure you need that `observeSpy` var if you properly indent the code ;)

::: shared/test/unit/mocks/mock_settings_listener.js
@@ +47,2 @@
>    }
>  };

please add 'use strict'; in this file

Comment 41

[[:fabrice] Fabrice Desré]

That makes me sad to solve this bug with a hidden setting. Brace yourself for abuse coming...

Comment 42

[Jonathan Griffin (:jgriffin)]

What if we enable this setting only on engineering/debug/marionette builds?

Comment 43

[Julien Wajsberg [:julienw]]

Jonathan: unless I'm mistaken, eng builds already disable the update subsystem and therefore don't have the notifications.

All: just got another idea. How about having a setting to disable the notification toaster for all notifications, not necessary only the update notifications. I know they actually disturb me most of the time, and therefore I would actually be happy to have an option to disable them.

And therefore we'd still have the notification in the utility tray.

Would this work for you Fabrice and Etienne ? And would this work for the automation team ?

Comment 44

[Zac C (:zac)]

(In reply to Julien Wajsberg [:julienw] from comment #43)
> Jonathan: unless I'm mistaken, eng builds already disable the update
> subsystem and therefore don't have the notifications.
> 

You are mistaken :(

> All: just got another idea. How about having a setting to disable the
> notification toaster for all notifications, not necessary only the update
> notifications. I know they actually disturb me most of the time, and
> therefore I would actually be happy to have an option to disable them.
> 
> And therefore we'd still have the notification in the utility tray.
> 
> Would this work for you Fabrice and Etienne ? And would this work for the
> automation team ?

We do have tests that check the notification toaster's functionality so we'd lose that coverage.

Let's see what Fabrice says about comment #42, it might make him less sad :)

Comment 45

[Bob Silverberg [:bsilverberg]]

If it is a setting to disable the notification toaster, then I assume we could use it to enable the toaster for just those tests that require it.

Comment 46

[Julien Wajsberg [:julienw]]

Fabrice, would comment 43 work better for you ?

Comment 47

[[:fabrice] Fabrice Desré]

Guys, I still think that all this enable/disable way is not optimal, but I also have no time to really help you with the xpcom override :( So do it like comment 42 for now, and file a followup to replace that with an xpcom override (feel free to assign to me).

Comment 48

[Etienne Segonzac (:etienne)]

Is this still blocking automation/QA?

With the download manager development going on and all, I'm starting to be more in favor of Julien's suggestion: a setting to disable to toaster altogether.

> All: just got another idea. How about having a setting to disable the
> notification toaster for all notifications, not necessary only the update
> notifications. I know they actually disturb me most of the time, and
> therefore I would actually be happy to have an option to disable them.

Comment 49

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(In reply to Etienne Segonzac (:etienne) from comment #48)
> Is this still blocking automation/QA?

This is still affecting the Eideticker automated tests as this popup will prevent us from detecting when a stable frame is achieved, and is also likely to be affecting FPS results.

I believe the functional UI tests work around this by interacting with elements at coordinates that avoid the popup, but Zac would be able to answer this with more certainty.

> With the download manager development going on and all, I'm starting to be
> more in favor of Julien's suggestion: a setting to disable to toaster
> altogether.

So long as we can enable it for the few tests that are checking the notification popup that works for me, however I feel it would be even better if we could prevent the device from even checking for updates.

Comment 50

[Zac C (:zac)]

Disabling the toaster does not seem to make sense; why disable one of the most important parts of the UI in favour of keeping a background operation running? It'd seem a bit like shooting off your nose to spite your face.

From a UI tests perspective we would prefer to keep the toaster active and just disable updates.

Comment 51

[Etienne Segonzac (:etienne)]

Make sense.

Resetting the assignee to reflect that it's ok to work on it.

Comment 52

[William Lachance (:wlach)]

We really still do need this fixed for Eideticker, the numbers we're producing won't be reliable until is. Could we get someone working on this please?

Comment 53

[Etienne Segonzac (:etienne)]

(In reply to William Lachance (:wlach) from comment #52)
> We really still do need this fixed for Eideticker, the numbers we're
> producing won't be reliable until is. Could we get someone working on this
> please?

We need someone with gecko skills to prevent the check, otherwise the network operation will kill the reliability anyway.

Jonathan? :)

Comment 54

[Jonathan Griffin (:jgriffin)]

I'm going to pass the buck to Marshall...Marshall, do you have any idea how we can accomplish this?

Comment 55

[Alexandre LISSY :gerard-majax]

After discussing with Fabrice on IRC, I will try to work on this.

Comment 56

[Alexandre LISSY :gerard-majax]

I'm testing some way to do it, and I need to know more about the way you run your tests: specific build flag ? Would something in gaia profile fit you to override the update checker component ?

Comment 57

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

We currently download the nightly eng builds and either use it with the Gaia profile that's already packaged, or we run make reset-gaia from the tip of the appropriate gaia branch. We'd need this to work for both scenarios, so ideally we don't want to have to build gaia.

Comment 58

[Alexandre LISSY :gerard-majax]

Okay, thanks to Fabrice and Vivien links regarding Gecko, and thanks to Dave and AutomatedTester help regarding Marionette, I think I have something that starts to match your needs:

I/Gecko   (  651): 1389976469402	Marionette	DEBUG	accepted connection on 127.0.0.1:33939
I/Gecko   (  651): 1389976469666	Marionette	INFO	loaded marionette-listener.js
I/Gecko   (  651): 1389976469668	Marionette	INFO	sendToClient: {"from":"0","value":"3-b2g"}, {4b6e7f33-d7b1-44be-98dc-692fdf99a3cf}, {4b6e7f33-d7b1-44be-98dc-692fdf99a3cf}
I/Gecko   (  651): 1389976469680	Marionette	DEBUG	Got request: setContext, data: {"to":"0","sessionId":"3-b2g","name":"setContext","parameters":{"value":"chrome"}}, id: {5a15afce-d186-410e-ba69-49cd54ca7338}
I/Gecko   (  651): 1389976469680	Marionette	INFO	sendToClient: {"from":"0","ok":true}, {5a15afce-d186-410e-ba69-49cd54ca7338}, {5a15afce-d186-410e-ba69-49cd54ca7338}
I/Gecko   (  651): 1389976469697	Marionette	INFO	sendToClient: {"from":"0","ok":true}, {6cd86f4f-10e3-4145-9102-320d6b85f013}, {6cd86f4f-10e3-4145-9102-320d6b85f013}
I/Gecko   (  651): 1389976469702	Marionette	DEBUG	Got request: execute, data: {"to":"0","sessionId":"3-b2g","name":"executeScript","parameters":{"specialPowers":false,"scriptTimeout":null,"newSandbox":true,"args":[],"filename":"fake_update-checker.py","script":"","line":9}}, id: {9f212f29-bce0-4342-b124-c0eedd3b3039}
I/GeckoDump(  651): FakeUpdateChecker: FakeUpdateChecker CID: {b4b297db-edff-4adc-9f09-87aa79ac7475}
I/GeckoDump(  651): FakeUpdateChecker: UpdateChecker CID: {898cdc9b-e43f-422f-9cc4-2f6291b415a3}
I/Gecko   (  651): 1389976469822	Marionette	INFO	sendToClient: {"from":"0","value":null}, {9f212f29-bce0-4342-b124-c0eedd3b3039}, {9f212f29-bce0-4342-b124-c0eedd3b3039}
I/Gecko   (  651): 1389976469835	Marionette	INFO	sendToClient: {"from":"0","ok":true}, {e01bdc87-4141-4add-9d5d-bc893d30d273}, {e01bdc87-4141-4add-9d5d-bc893d30d273}
I/Gecko   (  651): UpdatePrompt: Forcing update check
I/GeckoDump(  651): FakeUpdateChecker: createInstance: IN
I/GeckoDump(  651): FakeUpdateChecker: constructor
I/GeckoDump(  651): FakeUpdateChecker: createInstance: OUT
I/GeckoDump(  651): FakeUpdateChecker: checkForUpdates, force: true
I/Gecko   (  651): *** AUS:SVC UpdateManager:_loadXMLFileIntoArray: XML file does not exist
E/GeckoConsole(  651): AUS:SVC UpdateManager:_loadXMLFileIntoArray: XML file does not exist
I/Gecko   (  651): UpdatePrompt: Setting gecko.updateStatus: no-updates
I/GeckoDump(  651): XXX FIXME : Got a mozContentEvent: force-update-check
E/GeckoConsole(  800): Content JS ERROR at app://settings.gaiamobile.org/js/about.js:103 in onUpdateStatus: Error checking for system update: no-updates
I/Gecko   (  651): UpdatePrompt: Forcing update check
I/GeckoDump(  651): FakeUpdateChecker: createInstance: IN
I/GeckoDump(  651): FakeUpdateChecker: createInstance: OUT
I/GeckoDump(  651): FakeUpdateChecker: checkForUpdates, force: true
I/Gecko   (  651): UpdatePrompt: Setting gecko.updateStatus: no-updates
I/GeckoDump(  651): XXX FIXME : Got a mozContentEvent: force-update-check
E/GeckoConsole(  800): Content JS ERROR at app://settings.gaiamobile.org/js/about.js:103 in onUpdateStatus: Error checking for system update: no-updates
I/Gecko   (  651): UpdatePrompt: Forcing update check
I/GeckoDump(  651): FakeUpdateChecker: createInstance: IN
I/GeckoDump(  651): FakeUpdateChecker: createInstance: OUT
I/GeckoDump(  651): FakeUpdateChecker: checkForUpdates, force: true
I/Gecko   (  651): UpdatePrompt: Setting gecko.updateStatus: no-updates
I/GeckoDump(  651): XXX FIXME : Got a mozContentEvent: force-update-check
E/GeckoConsole(  800): Content JS ERROR at app://settings.gaiamobile.org/js/about.js:103 in onUpdateStatus: Error checking for system update: no-updates
I/Gecko   (  651): UpdatePrompt: Forcing update check
I/GeckoDump(  651): FakeUpdateChecker: createInstance: IN
I/GeckoDump(  651): FakeUpdateChecker: createInstance: OUT
I/GeckoDump(  651): FakeUpdateChecker: checkForUpdates, force: true
I/Gecko   (  651): UpdatePrompt: Setting gecko.updateStatus: no-updates
I/GeckoDump(  651): XXX FIXME : Got a mozContentEvent: force-update-check
E/GeckoConsole(  800): Content JS ERROR at app://settings.gaiamobile.org/js/about.js:103 in onUpdateStatus: Error checking for system update: no-updates
I/Gecko   (  651): UpdatePrompt: Forcing update check
I/GeckoDump(  651): FakeUpdateChecker: createInstance: IN
I/GeckoDump(  651): FakeUpdateChecker: createInstance: OUT
I/GeckoDump(  651): FakeUpdateChecker: checkForUpdates, force: true
I/Gecko   (  651): UpdatePrompt: Setting gecko.updateStatus: no-updates
I/GeckoDump(  651): XXX FIXME : Got a mozContentEvent: force-update-check
I/Gecko   (  651): UpdatePrompt: Forcing update check
I/GeckoDump(  651): FakeUpdateChecker: createInstance: IN
I/GeckoDump(  651): FakeUpdateChecker: createInstance: OUT
I/GeckoDump(  651): FakeUpdateChecker: checkForUpdates, force: true
I/Gecko   (  651): UpdatePrompt: Setting gecko.updateStatus: no-updates
I/GeckoDump(  651): XXX FIXME : Got a mozContentEvent: force-update-check
E/GeckoConsole(  800): Content JS ERROR at app://settings.gaiamobile.org/js/about.js:103 in onUpdateStatus: Error checking for system update: no-updates
E/GeckoConsole(  800): Content JS ERROR at app://settings.gaiamobile.org/js/about.js:103 in onUpdateStatus: Error checking for system update: no-updates
I/Gecko   (  651): UpdatePrompt: Forcing update check
I/GeckoDump(  651): FakeUpdateChecker: createInstance: IN
I/GeckoDump(  651): FakeUpdateChecker: createInstance: OUT
I/GeckoDump(  651): FakeUpdateChecker: checkForUpdates, force: true
I/Gecko   (  651): UpdatePrompt: Setting gecko.updateStatus: no-updates
I/GeckoDump(  651): XXX FIXME : Got a mozContentEvent: force-update-check
E/GeckoConsole(  800): Content JS ERROR at app://settings.gaiamobile.org/js/about.js:103 in onUpdateStatus: Error checking for system update: no-updates

Comment 59

[Alexandre LISSY :gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/15469

This PR is WIP for now. It works but it's not integrated into Gaia UI Tests yet.

Comment 60

[Alexandre LISSY :gerard-majax]

Okay, I fixed a couple of things, there's a travis build running at https://travis-ci.org/mozilla-b2g/gaia/builds/17142402

I have good hope that it will work.

Comment 61

[Alexandre LISSY :gerard-majax]

I have something that seems to pass tests locally now, but I fear the failure were because I'm registering too early and the original update-checker service was not yet registered.

Zac, can you check if I can register later in the Python code ?

Fabrice, is there any risk if I register before the original update-checker service ?

Comment 62

[Alexandre LISSY :gerard-majax]

Travis build https://travis-ci.org/mozilla-b2g/gaia/jobs/17147783 seems to be okay and enables the fake update-checker service, as of https://github.com/mozilla-b2g/gaia/commit/2bf664f5571e26c01b35c1d5ba322e4b1ead245b

Comment 63

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Attachment: logcat-fakeupdate.txt

I've just tried with the patch applied and unfortunately I still see the update notification toaster. I've attached the output for: adb logcat | grep FakeUpdateChecker.

Comment 64

[Alexandre LISSY :gerard-majax]

(In reply to Dave Hunt (:davehunt) from comment #63)
> Created attachment 8361877 [details]
> logcat-fakeupdate.txt
> 
> I've just tried with the patch applied and unfortunately I still see the
> update notification toaster. I've attached the output for: adb logcat | grep
> FakeUpdateChecker.

I/GeckoDump(20007): FakeUpdateChecker: checkForUpdates, force: true

So the FakeUpdateChecker did his job.

Could you please document better STRs for this ?

Comment 65

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

The best way to replicate this is to run the Gaia UI tests against a device. Running a test such as functional/browser/test_browser_cell_data.py will replicate it. If you include the --restart command then you will see the device restart, and shortly after the cell data is enabled by the test, the update notification banner will pop down from the top of the device's screen.

Comment 66

[Alexandre LISSY :gerard-majax]

Can you include full logcat ?

Comment 67

[Alexandre LISSY :gerard-majax]

As far as I understand, update-checker service is for system updates (Gecko+Gaia, distributed as MAR packages). The more I read this bug the more I understand that you are talking about Webapps updates.

Those are triggered by Webapps.updateApps() code, launched upon a 'update-check-start' message, which is sent by the update-checker service |Services.obs.notifyObservers(null, "update-check-start", null);| right in the start of checkForUpdates() method.

Comment 68

[Julien Wajsberg [:julienw]]

Yep, you actually need to block both update systems.

Comment 69

[Alexandre LISSY :gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #68)
> Yep, you actually need to block both update systems.

My point is, if update-checker is replaced (and it seems to be), I see no way that the update-check-start event can be triggered.

Comment 70

[Alexandre LISSY :gerard-majax]

I just spent the last one and half hour trying to reproduce but I'm unable to. I don't even see any update checking being performed.

Comment 71

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(In reply to Alexandre LISSY :gerard-majax from comment #67)
> As far as I understand, update-checker service is for system updates
> (Gecko+Gaia, distributed as MAR packages). The more I read this bug the more
> I understand that you are talking about Webapps updates.

Yes, as far as I know the nightly engineering builds we use in automation are not offered gecko/gaia updates. This bug is only concerning the app update notifications.

Comment 72

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Steps to reproduce:

1. Download and flash the latest nightly engineering build to your device. The easiest way to do this is (for hamachi):
   a. git clone https://github.com/Mozilla-TWQA/B2G-flash-tool.git
   b. cd B2G-flash-tool
   c. ./auto_flash_from_PVT.sh -v master -d hamachi -g -G --eng

2. Install gaiatest into a Python environment (I recommend a virtual environment)
   a. virtualenv bug879192
   b. source bug879192/bin/activate
   c. pip install gaiatest

3. adb forward tcp:2828 tcp:2828

4. gcli unlock

5. gcli killapps

6. Connect to a WiFi network (replace options with your desired network):
   a. gcli connectwifi [--security {WPA-PSK,WEP}] [--password PASSWORD] ssid

Then watch for the update notification. If you want to replicate again, run:

1. adb shell stop b2g
2. adb shell start b2g

Then watch for the update notification again (it will even appear on the lockscreen).

Comment 73

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Attachment: logcat.txt

Comment 74

[Alexandre LISSY :gerard-majax]

Thanks, those STRs might explain the behavior then: b2g/components/WebappsUpdateTimer.js registers a timer and an observer against online/offline status. Upon online state, it will trigger a WebappsUpdater.updateApps() call. I've updated my PR to include some faking of the update timer.

As far as I can say, once applied, I can't reproduce.

Comment 75

[Alexandre LISSY :gerard-majax]

Pending travis: https://travis-ci.org/mozilla-b2g/gaia/builds/17268279

Comment 76

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Attachment: passing logcat.txt

The latest version of the patch works for me! I've attached the logcat of the test running without the notification.

Comment 77

[Alexandre LISSY :gerard-majax]

PR updated for some cleanup, travis green https://travis-ci.org/mozilla-b2g/gaia/builds/17280908

Dave, can you cross check it still works?

Comment 78

[Alexandre LISSY :gerard-majax]

Comment on attachment 8361770 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15469

This is working quite well, time for a review :).

Comment 79

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Attachment: failing logcat.txt

Unfortunately the latest version of the patch does not appear to be working. I've attached a full logcat.

Comment 80

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Attachment: failing logcat.txt

Comment 81

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

Not sure what was causing me to see the update notification earlier. I've just reflashed my device and ran again with the latest patch and I don't see it. Thanks Alexandre!

Comment 82

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(In reply to Dave Hunt (:davehunt) from comment #81)
> Not sure what was causing me to see the update notification earlier. I've
> just reflashed my device and ran again with the latest patch and I don't see
> it. Thanks Alexandre!

It's perhaps due to previously running without the patch and not reflashing/resetting between runs. Could be that the update notification is cached somewhere that we're not clearing up between tests. This shouldn't matter as we'll be running this code immediately after a flash in automation, but it would be good to know where this cache is so we can clean it up too.

Comment 83

[Alexandre LISSY :gerard-majax]

After discussing this topic with :vingtetun, :etienne and :Rik we indeed have some code that does this kind of caching in Gaia.

Nice to know that finally we have it working.

Comment 84

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8361770 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15469

Thanks, gaia_test.py changes look good. I just had some feedback about the fake_update-checker.js's FUC_getUpdateURL function which is in the PR.

Comment 85

[[:fabrice] Fabrice Desré]

Comment on attachment 8361770 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15469

I'd like to take a second look after nits are fixed.

Comment 86

[Alexandre LISSY :gerard-majax]

(In reply to Fabrice Desré [:fabrice] from comment #85)
> Comment on attachment 8361770 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/15469
> 
> I'd like to take a second look after nits are fixed.

Of course. Nits are addressed, I'll recheck after lunch that it works as expected. Thanks for the feedback

Comment 87

[Alexandre LISSY :gerard-majax]

When I run the test_browser_cell_data.py test on my Inari device which has WiFi already configured, I have to hack the test otherwise WiFi gets used and makes the test failing.

Would this need that we open a followup ?

Comment 88

[Alexandre LISSY :gerard-majax]

Comment on attachment 8361770 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15469

I tested the new PR on my Inari device, it seems the behavior is correct. So all nits are addressed, I'm switching back the review flag :).

Comment 89

[Alexandre LISSY :gerard-majax]

Travis is green: https://travis-ci.org/mozilla-b2g/gaia/builds/17362595

Comment 90

[Alexandre LISSY :gerard-majax]

PR rebased on master, I'll wait for green travis regarding the recent jshint changes: https://travis-ci.org/mozilla-b2g/gaia/builds/17462967.

Comment 91

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(In reply to Alexandre LISSY :gerard-majax from comment #87)
> When I run the test_browser_cell_data.py test on my Inari device which has
> WiFi already configured, I have to hack the test otherwise WiFi gets used
> and makes the test failing.
> 
> Would this need that we open a followup ?

The test setUp should take care of disabling WiFi and forgetting all known networks. See https://github.com/mozilla-b2g/gaia/blob/d727014d8837e3ddf09be5b1f2496ca1127b65c1/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L928 for a cleaner restart you should include the --restart command line option, which removed persistent storage and the profile.

Comment 92

[Alexandre LISSY :gerard-majax]

Green travis: https://travis-ci.org/mozilla-b2g/gaia/builds/17462967

Comment 93

[Alexandre LISSY :gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/b4be9d9881329e4c5b80788c9bdcf7af4032480b