Closed Bug 879192 Opened 9 years ago Closed 8 years ago

Add preference/setting for disabling app update notifications

Categories

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

Other
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)

People

(Reporter: zcampbell, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(6 files, 1 obsolete file)

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!
Component: General → Gaia::System
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.
Flags: needinfo?(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?
Flags: needinfo?(etienne)
Flags: needinfo?(zcampbell)
I currently see:

2 updates available
Marketplace, 27.61 kB
PackStubTest, 5.83 kB
Flags: needinfo?(zcampbell)
Flags: needinfo?(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?
Flags: needinfo?(etienne)
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.
Flags: needinfo?(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.
Flags: needinfo?(etienne)
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.
(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.
Flags: needinfo?(etienne)
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.
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.
(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.
(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.
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.
Flags: needinfo?(etienne)
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.
Assignee: nobody → etienne
Summary: Unable to suppress app update notifications in test automation → Add preference/setting for disabling app update notifications
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.
Blocks: 916989
Flags: needinfo?(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.
Flags: needinfo?(etienne)
Attached patch Patch proposalSplinter Review
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?
Attachment #806684 - Flags: review?(felash)
Attachment #806684 - Flags: feedback?(zcampbell)
Etienne I'm not in a position to self build right now.

Can anybody else test it for me?
Maybe Dave?
Flags: needinfo?(dave.hunt)
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 ?
Attachment #806684 - Flags: feedback?
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 ?
Flags: needinfo?(fabrice)
I know eng builds don't have updates at all. Could be another idea to make your tests work ?
Attachment #806684 - Flags: feedback?
Engineering builds do get updates; see comments #3 - #5
(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.
Flags: needinfo?(fabrice)
> 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.
Flags: needinfo?(dave.hunt)
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.
Zac, would Fabrice's solution work?
Flags: needinfo?(zcampbell)
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.
Flags: needinfo?(fabrice)
+1 to that Julien, Bebe spent a bit of time on it yesterday but needs a bit of help too!
Flags: needinfo?(zcampbell)
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
Flags: needinfo?(fabrice)
What's the status here?
Can we confirm the solution from Comment 26?
missing ni
Flags: needinfo?(zcampbell)
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.
Flags: needinfo?(zcampbell)
Hey, we're trying to see if we can do the solution proposed on Comment 26, any idea?
Flags: needinfo?(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
Flags: needinfo?(jgriffin) → needinfo?(fabrice)
Yes, that's the idea!
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
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.
(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.).
Flags: needinfo?(fabrice)
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
Attachment #806684 - Flags: review?(felash)
That makes me sad to solve this bug with a hidden setting. Brace yourself for abuse coming...
What if we enable this setting only on engineering/debug/marionette builds?
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 ?
(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 :)
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.
Fabrice, would comment 43 work better for you ?
Flags: needinfo?(fabrice)
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).
Flags: needinfo?(fabrice)
Attachment #806684 - Flags: feedback?(zcampbell)
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.
(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.
Flags: needinfo?(zcampbell)
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.
Flags: needinfo?(zcampbell)
Make sense.

Resetting the assignee to reflect that it's ok to work on it.
Assignee: etienne → nobody
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?
Flags: needinfo?(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? :)
Flags: needinfo?(etienne)
I'm going to pass the buck to Marshall...Marshall, do you have any idea how we can accomplish this?
Flags: needinfo?(marshall)
Whiteboard: [systemsfe]
After discussing with Fabrice on IRC, I will try to work on this.
Assignee: nobody → lissyx+mozillians
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 ?
Flags: needinfo?(zcampbell)
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.
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
Flags: needinfo?(zcampbell)
This PR is WIP for now. It works but it's not integrated into Gaia UI Tests yet.
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.
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 ?
Flags: needinfo?(zcampbell)
Flags: needinfo?(fabrice)
Attached file 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.
(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 ?
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.
Can you include full logcat ?
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.
Yep, you actually need to block both update systems.
(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.
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.
(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.
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).
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.
Flags: needinfo?(dave.hunt)
Attached file passing logcat.txt
The latest version of the patch works for me! I've attached the logcat of the test running without the notification.
Flags: needinfo?(dave.hunt)
PR updated for some cleanup, travis green https://travis-ci.org/mozilla-b2g/gaia/builds/17280908

Dave, can you cross check it still works?
Flags: needinfo?(dave.hunt)
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 :).
Attachment #8361770 - Flags: review?(mdas)
Attachment #8361770 - Flags: review?(fabrice)
Attached file failing logcat.txt (obsolete) —
Unfortunately the latest version of the patch does not appear to be working. I've attached a full logcat.
Flags: needinfo?(dave.hunt)
Attached file failing logcat.txt
Attachment #8362881 - Attachment is obsolete: true
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!
(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.
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 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.
Attachment #8361770 - Flags: review?(mdas) → review+
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.
Attachment #8361770 - Flags: review?(fabrice) → feedback+
Flags: needinfo?(fabrice)
(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
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 ?
Flags: needinfo?(dave.hunt)
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 :).
Attachment #8361770 - Flags: review?(fabrice)
Attachment #8361770 - Flags: review?(fabrice) → review+
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
PR rebased on master, I'll wait for green travis regarding the recent jshint changes: https://travis-ci.org/mozilla-b2g/gaia/builds/17462967.
(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.
Flags: needinfo?(dave.hunt)
https://github.com/mozilla-b2g/gaia/commit/b4be9d9881329e4c5b80788c9bdcf7af4032480b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Flags: needinfo?(zcampbell)
Flags: needinfo?(marshall)
Depends on: 1094362
You need to log in before you can comment on or make changes to this bug.