794937 - Work - Updates should still be applied if a user only uses the Metro browser

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

Background: Updates are currently enabled on desktop and disabled in Metro.  Both use a different update directory as well so the Metro browser should never try to apply the update of the Desktop browser.

This bug is to find a way to make sure users will still get updates even if they only use the Metro browser.

Possible solutions:
- Do nothing, users that only use the Metro browser will probably eventually use the Desktop, it may be several weeks though and we would have to live with this.
- Combine both update directories into one and enable updates in both the Metro profile and the Desktop profile.  Add synchronization to make sure there are no race conditions. 
- Have the metro browser detect if there is an update pending and silently launch the Desktop browser to do that update.  Or just silently launch the Desktop browser periodically once every week or so to check for any updates.

Comment 1

[Brian R. Bondy [:bbondy]]

CCing rstrong for feedback, no rush.

Comment 2

[Brian R. Bondy [:bbondy]]

Had a 1:1 with rstrong and discussed this bug.

The plan is to:
- Enable updates on the Metro side
- Share the Firefox update folder which contains update.status and update.mar
- Create a named mutex for the duration of the download of the update
- Make sure both processes do not apply an update at the exact same time (possible race condition)
- Allow firefox.exe to be renamed like the other dlls in the to_be_deleted folder
- (Might or might not be needed because of the previous point) When starting the Metro browser, bypass the apply update if the desktop browser is opened.

Out of scope for this bug but that we'll want to investigate later in another bug:
- Providing some kind of UI for updates on the Metro side for 0 day attacks.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Are you talking about two different processes that might download the same update or different ones ? What requirements will there be (if any) on RelEng for separate packaging and serving of updates ?

Comment 4

[Brian R. Bondy [:bbondy]]

> Are you talking about two different processes that might download the same update or different ones ?

2 different processes that might download the same update. 

> What requirements will there be (if any) on RelEng for separate packaging and serving of updates ?

There are none in relation to this bug.   There will be only 1 update package and it will be served from the same channel that already exists. 

There are build requirements for the Win8SDK and test machine setup, but that is not part of this bug and I believe other people are working on that.

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

That's great to hear, thanks for the early xmas pressie. :-)

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1. toolkit/mozapps/update

Just wanted to get thoughts on this part of the code so far.

There's a couple of other patches still coming with:
1) Toolkit code on startup to determine if an update should be applied or not (because another instance is already open)
2) Code in updater to relaunch the application in metro

Note that in another bug we'll be adding updates to the about flyout in Metro but that's not in this bug.

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 736052 [details] [diff] [review]
Patch v1. toolkit/mozapps/update

Wrong patch :)

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1' - update js and about dialog code

The premise of the patch is that users can run more than one instance from the same path.  This was somewhat uncommon before with using different profiles and -no-remote.  This is now very common with Metro and Firefox on Desktop.  So the patch makes the first instance used to be the one in charge of downloading and applying the update. 

Downloads and updates for both Metro and Desktop go to the same path as per bug 572162's patch.

See Comment 7 for details on the other patches coming.

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 736101 [details] [diff] [review]
Patch v1' - update js and about dialog code

Looks like one of the approaches we discussed and I'm fine with this. In an ideal world this would handle not only -no-remote for the same user as well as multiple users trying to update Firefox. Let's discuss this over vidyo on Friday as well.

Comment 11

[Brian R. Bondy [:bbondy]]

This is also handling multiple users logged in on different sessions using the same browser because I'm using a Global\\ prefix on the mutex name.  It's not obvious from the patch because it's a boolean parameter. I'm not sure yet if it's the best way to go because a limited user account could open first locking the admin out from doing updates. I think it's the right way to go but we should do the limited user account updating through the service really soon.

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Updater callback

After updates we need to relaunch the Metro browser, this adds special handling to the updater to do that. It is mostly the same as the delegate execute handler and the metro test harness launching.

Comment 13

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #11)
> This is also handling multiple users logged in on different sessions using
> the same browser because I'm using a Global\\ prefix on the mutex name. 
> It's not obvious from the patch because it's a boolean parameter. I'm not
> sure yet if it's the best way to go because a limited user account could
> open first locking the admin out from doing updates. I think it's the right
> way to go but we should do the limited user account updating through the
> service really soon.
Great! One thing to note though is that if a user is not doing an update which is the case for any account that can't update then the mutex shouldn't be created to handle that case properly.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 736345 [details] [diff] [review]
Patch v1 - Updater callback

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

::: toolkit/mozapps/update/common/updatehelper.cpp
@@ +10,5 @@
> +#define _WIN32_WINNT 0x602
> +#include <objbase.h>
> +#include <shobjidl.h>
> +#pragma comment(lib, "ole32.lib")
> +#endif

Outside of our "special place" in widget this is a little concerning. But I suppose if everything builds up right it's ok.

Note, #ifdef MOZ_METRO doesn't necessarily imply XP_WIN, although I don't think unix support is being maintained. *shrug*

@@ +733,5 @@
> +
> +    HKEY key;
> +    if (RegOpenKeyExW(HKEY_CLASSES_ROOT, kDefaultMetroBrowserIDPathKey,
> +                      0, KEY_READ, &key) != ERROR_SUCCESS) {
> +      // XXX TODO: I tink we can remove kDemoMetroBrowserIDPathKeyp check

We don't use this anymore. Is this being used someplace else? If so it should go away as well.

@@ +756,5 @@
> +  {
> +    CoInitialize(NULL);
> +    HRESULT hr = E_FAIL;
> +    // The interface that allows us to activate the browser
> +    IApplicationActivationManager *activateMgr;

Do we have access to nsRefPtr or similar in here? If so might want to use it for this.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +1663,5 @@
>  //-----------------------------------------------------------------------------
>  
>  #ifdef XP_WIN
>  #include "nsWindowsRestart.cpp"
> +#include "nsWindowsHelpers.h"

Did we need this?

Comment 15

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> (In reply to Brian R. Bondy [:bbondy] from comment #11)
> > This is also handling multiple users logged in on different sessions using
> > the same browser because I'm using a Global\\ prefix on the mutex name. 
> > It's not obvious from the patch because it's a boolean parameter. I'm not
> > sure yet if it's the best way to go because a limited user account could
> > open first locking the admin out from doing updates. I think it's the right
> > way to go but we should do the limited user account updating through the
> > service really soon.
> Great! One thing to note though is that if a user is not doing an update
> which is the case for any account that can't update then the mutex shouldn't
> be created to handle that case properly.

Good point and yup the patch acquires the mutex only after the other checks pass.

Comment 16

[Brian R. Bondy [:bbondy]]

Thanks for taking an early look Jim! (And Robert for the other patch)

(In reply to Jim Mathies [:jimm] from comment #14)
> Comment on attachment 736345 [details] [diff] [review]
> Patch v1 - Updater callback
> 
> Review of attachment 736345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/common/updatehelper.cpp
> @@ +10,5 @@
> > +#define _WIN32_WINNT 0x602
> > +#include <objbase.h>
> > +#include <shobjidl.h>
> > +#pragma comment(lib, "ole32.lib")
> > +#endif
> 
> Outside of our "special place" in widget this is a little concerning. But I
> suppose if everything builds up right it's ok.

Ya I'm not sure if we could build otherwise. 
 
> Note, #ifdef MOZ_METRO doesn't necessarily imply XP_WIN, although I don't
> think unix support is being maintained. *shrug*

Yup I know but this is a Windows only file. 

> We don't use this anymore. Is this being used someplace else? If so it
> should go away as well.

Ya it's being used in metro test harness launching. I'll remove it here and there.

> 
> @@ +756,5 @@
> > +  {
> > +    CoInitialize(NULL);
> > +    HRESULT hr = E_FAIL;
> > +    // The interface that allows us to activate the browser
> > +    IApplicationActivationManager *activateMgr;
> 
> Do we have access to nsRefPtr or similar in here? If so might want to use it
> for this.

No I don't think we can use it in the updater application, but I would have liked to use it.

> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +1663,5 @@
> >  //-----------------------------------------------------------------------------
> >  
> >  #ifdef XP_WIN
> >  #include "nsWindowsRestart.cpp"
> > +#include "nsWindowsHelpers.h"
> 
> Did we need this?

Yup we need it for the call to LaunchDefaultMetroBrowser that this patch introduced.

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Updater callback

Same as before but removed references to Mozilla.Firefox.URL

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Updater callback

Now add in immersive callback arg manually since it's not automatically included in the updater callback arguments and we need it to determine which browser to use.

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Allow callback app to be renamed if in use

Comment 20

[Brian R. Bondy [:bbondy]]

I'll probably move that last patch out to the intermittent test failure bug if it works.

Comment 21

[Brian R. Bondy [:bbondy]]

bug 715746

Comment 22

[Brian R. Bondy [:bbondy]]

Comment on attachment 736817 [details] [diff] [review]
Patch v1 - Allow callback app to be renamed if in use

Moved this to bug 715746

Comment 23

[Brian R. Bondy [:bbondy]]

Comment on attachment 736101 [details] [diff] [review]
Patch v1' - update js and about dialog code

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

This is working from testing locally so marking for review

Comment 24

[Brian R. Bondy [:bbondy]]

Comment on attachment 736642 [details] [diff] [review]
Patch v3 - Updater callback

This is working from testing locally.
jimm feedback+'ed the launch in metro browser related code specifically.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - update js and about dialog code

Fixed minor bug for multi installs where 2 instances were allowed to download updates at the same time. Fix involved was something to the effect of:

if (handle.isNull())
   handle = null;

(i.e. before we had a check like if (handle) and that doesn't' work.

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Fix metro updater url

Kind of a hack, I'd prefer server side aliases MetroFirefox for Firefox in this URL:

https://aus3.mozilla.org/update/3/MetroFirefox/23.0a1/20130423135252/WINNT_x86-msvc/en-US/nightly-oak/Windows_NT%206.2.0.0%20(x64)/default/default/update.xml?force=1

->

https://aus3.mozilla.org/update/3/Firefox/23.0a1/20130423135252/WINNT_x86-msvc/en-US/nightly-oak/Windows_NT%206.2.0.0%20(x64)/default/default/update.xml?force=1

But that may take a while.

Comment 27

[Brian R. Bondy [:bbondy]]

Services.appInfo.UAName does hold "Firefox" by the way, but I think that's for user agent name purposes and I don't think it should be linked to this in case it changes.

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 736642 [details] [diff] [review]
Patch v3 - Updater callback

>diff --git a/toolkit/mozapps/update/common/updatehelper.h b/toolkit/mozapps/update/common/updatehelper.h
>--- a/toolkit/mozapps/update/common/updatehelper.h
>+++ b/toolkit/mozapps/update/common/updatehelper.h
>@@ -26,8 +26,13 @@ BOOL IsUnpromptedElevation(BOOL &isUnpro
> // The test only fallback key, as its name implies, is only present on machines
> // that will use automated tests.  Since automated tests always run from a 
> // different directory for each test, the presence of this key bypasses the
> // "This is a valid installation directory" check.  This key also stores
> // the allowed name and issuer for cert checks so that the cert check
> // code can still be run unchanged.
> #define TEST_ONLY_FALLBACK_KEY_PATH \
>   BASE_SERVICE_REG_KEY L"\\3932ecacee736d366d6436db0f55bce4"
>+
>+#ifdef MOZ_METRO
>+  bool GetDefaultBrowserAppModelID(WCHAR* aIDBuffer, long aCharLength);
>+  HRESULT LaunchDefaultMetroBrowser();
>+#endif
>\ No newline at end of file
nit: add a newline

Comment 29

[Robert Strong (they/them - no direct email)]

Comment on attachment 741447 [details] [diff] [review]
Patch v1 - Fix metro updater url

nthomas, can I get you to weigh in as to whether MetroFirefox can be aliased to Firefox?

bbondy, I am a tad concerned about the other cases we use Services.appinfo.name... have you looked at the other cases where it is used? Clearing review until after both nthomas and you respond.

Comment 30

[Robert Strong (they/them - no direct email)]

Nick, can I get your feedback on comment #29?

Comment 31

[Robert Strong (they/them - no direct email)]

Comment on attachment 736971 [details] [diff] [review]
Patch v2 - update js and about dialog code

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
>@@ -115,20 +115,20 @@ pref("app.update.cert.maxErrors", 5);
> //    the value for the name must be the same as the value for the attribute name
> //    on the certificate.
> // If these conditions aren't met it will be treated the same as when there is
> // no update available. This validation will not be performed when the
> // |app.update.url.override| user preference has been set for testing updates or
> // when the |app.update.cert.checkAttributes| preference is set to false. Also,
> // the |app.update.url.override| preference should ONLY be used for testing.
> pref("app.update.certs.1.issuerName", "OU=Equifax Secure Certificate Authority,O=Equifax,C=US");
>-pref("app.update.certs.1.commonName", "aus3.mozilla.org");
>+pref("app.update.certs.1.commonName", "aus3.mozilla.org"); // See also metro.js when updating these
> 
> pref("app.update.certs.2.issuerName", "CN=Thawte SSL CA,O=\"Thawte, Inc.\",C=US");
>-pref("app.update.certs.2.commonName", "aus3.mozilla.org");
>+pref("app.update.certs.2.commonName", "aus3.mozilla.org"); // See also metro.js when updating these
metro.js should be updated when certs.X.issuerName is updated as well so just add the note to the comment section above. Might be good to start the note with IMPORTANT! or something similar.

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js
>--- a/browser/base/content/aboutDialog.js
>+++ b/browser/base/content/aboutDialog.js
>@@ -116,16 +116,21 @@ function appUpdater()
>   }
> 
>   if (this.isPending || this.isApplied) {
>     this.setupUpdateButton("update.restart." +
>                            (this.isMajor ? "upgradeButton" : "updateButton"));
>     return;
>   }
> 
>+  if (!this.aus.isResponsibleForUpdates) {
Not sure about naming here and else where... I'll think about it.

>diff --git a/browser/locales/en-US/chrome/browser/aboutDialog.dtd b/browser/locales/en-US/chrome/browser/aboutDialog.dtd
>--- a/browser/locales/en-US/chrome/browser/aboutDialog.dtd
>+++ b/browser/locales/en-US/chrome/browser/aboutDialog.dtd
>@@ -42,16 +42,18 @@
> <!-- LOCALIZATION NOTE (update.checkingForUpdates): try to make the localized text short (see bug 596813 for screenshots). -->
> <!ENTITY update.checkingForUpdates  "Checking for updatesâ¦">
> <!-- LOCALIZATION NOTE (update.checkingAddonCompat): try to make the localized text short (see bug 596813 for screenshots). -->
> <!ENTITY update.checkingAddonCompat "Checking Add-on compatibilityâ¦">
> <!-- LOCALIZATION NOTE (update.noUpdatesFound): try to make the localized text short (see bug 596813 for screenshots). -->
> <!ENTITY update.noUpdatesFound      "&brandShortName; is up to date">
> <!-- LOCALIZATION NOTE (update.adminDisabled): try to make the localized text short (see bug 596813 for screenshots). -->
> <!ENTITY update.adminDisabled       "Updates disabled by your system administrator">
>+<!-- LOCALIZATION NOTE (update.notResponsible): try to make the localized text short (see bug 596813 for screenshots). -->
Remove " (see bug 596813 for screenshots)" since that bug does not have a screenshot for this otherwise this will cause confusion and complaints from localizers.

>+<!ENTITY update.notResponsible      "&brandShortName; is being updated by another instance">
I this this string is fine but you need to get UX r+ on this string.

Comment 32

[Robert Strong (they/them - no direct email)]

One thing that will bite some people with metro is that there will be no incompatibility check for add-ons in the non metro environment when performing an app update. Please bring this up to Asa, etc. It seems to me that it would be better to switch to native to perform an update to get around this issue but I will leave this to product and UX.

Comment 33

[Robert Strong (they/them - no direct email)]

Comment on attachment 736971 [details] [diff] [review]
Patch v2 - update js and about dialog code

>diff --git a/browser/metro/base/content/browser-ui.js b/browser/metro/base/content/browser-ui.js
>--- a/browser/metro/base/content/browser-ui.js
>+++ b/browser/metro/base/content/browser-ui.js
>@@ -148,22 +148,16 @@ var BrowserUI = {
>         PdfJs.init();
> #ifdef MOZ_SERVICES_SYNC
>         WeaveGlue.init();
> #endif
>       } catch(ex) {
>         Util.dumpLn("Exception in delay load module:", ex.message);
>       }
> 
>-#ifdef MOZ_UPDATER
>-      // Check for updates in progress
>-      let updatePrompt = Cc["@mozilla.org/updates/update-prompt;1"].createInstance(Ci.nsIUpdatePrompt);
>-      updatePrompt.checkForUpdates();
>-#endif
>-
What is the UX here? Is an update check performed on open?

Comment 34

[Robert Strong (they/them - no direct email)]

Comment on attachment 736971 [details] [diff] [review]
Patch v2 - update js and about dialog code

>diff --git a/browser/metro/components/components.manifest b/browser/metro/components/components.manifest
>--- a/browser/metro/components/components.manifest
>+++ b/browser/metro/components/components.manifest
>@@ -77,15 +77,9 @@ contract @mozilla.org/satchel/form-autoc
> component {97d12931-abe2-11df-94e2-0800200c9a66} LoginManagerPrompter.js
> contract @mozilla.org/login-manager/prompter;1 {97d12931-abe2-11df-94e2-0800200c9a66}
> 
> #ifdef MOZ_SAFE_BROWSING
> # SafeBrowsing.js
> component {aadaed90-6c03-42d0-924a-fc61198ff283} SafeBrowsing.js
> contract @mozilla.org/safebrowsing/application;1 {aadaed90-6c03-42d0-924a-fc61198ff283}
> category app-startup SafeBrowsing service,@mozilla.org/safebrowsing/application;1
>-#endif
>-
>-#ifdef MOZ_UPDATER
>-# UpdatePrompt.js
>-component {88b3eb21-d072-4e3b-886d-f89d8c49fe59} UpdatePrompt.js
>-contract @mozilla.org/updates/update-prompt;1 {88b3eb21-d072-4e3b-886d-f89d8c49fe59}
>-#endif
>+#endif
>\ No newline at end of file
nit: ^^

Comment 35

[Robert Strong (they/them - no direct email)]

Comment on attachment 736971 [details] [diff] [review]
Patch v2 - update js and about dialog code

>diff --git a/browser/metro/profile/metro.js b/browser/metro/profile/metro.js
>--- a/browser/metro/profile/metro.js
>+++ b/browser/metro/profile/metro.js
>@@ -405,29 +405,118 @@ pref("browser.search.param.yahoo-fr", "m
> pref("browser.search.param.yahoo-fr-cjkt", "moz35");
> pref("browser.search.param.yahoo-fr-ja", "mozff");
> #endif
> 
> /* app update prefs */
> pref("app.update.timer", 60000); // milliseconds (1 min)
This pref is no longer used and metro.js likely has several values that are different due to it originating in fennec. Please at least audit the app.update.* prefs for this bug and followup on the other ones.

Comment 36

[Robert Strong (they/them - no direct email)]

metro doesn't have its own firefox-branding.js... either it needs its own or it needs to use the one used by non metro Firefox.

Are there plans on making it possible to share a common pref file between metro and non metro Firefox?

Comment 37

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #29)
> Comment on attachment 741447 [details] [diff] [review]
> Patch v1 - Fix metro updater url
> 
> nthomas, can I get you to weigh in as to whether MetroFirefox can be aliased
> to Firefox?

That seems fine, given comment #4 says we want to serve the same file regardless of desktop or metro making the request. If you prefer to retain flexibility we could alias on the server side fairly easily. We already have support for that for Mac universal builds, given they might be running 32 or 64 bit.

driveby comment: metro.js is setting |pref("app.update.log", true);|

Comment 38

[Jim Mathies [:jimm]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #36)
> metro doesn't have its own firefox-branding.js... either it needs its own or
> it needs to use the one used by non metro Firefox.

Actually it does, we walk across browser/branding/(build) twice, once for each front end. 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/Makefile.in#20

Comment 39

[Jim Mathies [:jimm]]

If we need changes based on the build, we could pre-process the existing js file, or add a new js file in each branding/pref folder for metro and select the right one from the makefile based on DIST_SUBDIR.

Comment 40

[Robert Strong (they/them - no direct email)]

jimm, good to know and thanks!

Comment 41

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> One thing that will bite some people with metro is that there will be no
> incompatibility check for add-ons in the non metro environment when
> performing an app update. Please bring this up to Asa, etc. It seems to me
> that it would be better to switch to native to perform an update to get
> around this issue but I will leave this to product and UX.

I don't fully understand what you mean here. Won't there be an incompatible check on the next startup of the Desktop browser? Are you suggesting to not have updates in Metro at all?  Note there is no support for add-ons in the Metro browser at all.  What is the current handling in Desktop? Do we ever "not take in an update" when it may cause an incompatible update? If so, in which cases do we do that, and couldn't we just apply the same logic in Metro?

> It seems to me
> that it would be better to switch to native to perform an update to get
> around this issue

Could you expand on this suggestion more?

Comment 42

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #41)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> > One thing that will bite some people with metro is that there will be no
> > incompatibility check for add-ons in the non metro environment when
> > performing an app update. Please bring this up to Asa, etc. It seems to me
> > that it would be better to switch to native to perform an update to get
> > around this issue but I will leave this to product and UX.
> 
> I don't fully understand what you mean here. Won't there be an incompatible
> check on the next startup of the Desktop browser? Are you suggesting to not
> have updates in Metro at all?  Note there is no support for add-ons in the
> Metro browser at all.  What is the current handling in Desktop? Do we ever
> "not take in an update" when it may cause an incompatible update? If so, in
> which cases do we do that, and couldn't we just apply the same logic in
> Metro?
App update calls the add-ons manager to get a list of installed add-ons and informs the user which add-ons are not compatible with the update. When in the metro browser the desktop browser add-ons won't be returned and the update will continue. When in the desktop browser the desktop browser add-ons will be returned and app update will call the add-ons manager to check if there are updates for the add-ons that aren't compatible with the update.

> > It seems to me
> > that it would be better to switch to native to perform an update to get
> > around this issue
> 
> Could you expand on this suggestion more?
If the metro browser checked for updates and instead launched the desktop browser when an update was found then the desktop add-ons would be checked for compatibility, etc.

Comment 43

[Brian R. Bondy [:bbondy]]

> App update calls the add-ons manager to get a list of installed add-ons 
> and informs the user which add-ons are not compatible with the update. 

Does this happen even if the user goes to the about dialog to do the update? Or only on the update timer check?

Do we ever force the update for incompatible addons?

> When in the metro browser the desktop browser add-ons won't be returned 
> and the update will continue. When in the desktop browser the desktop 
> browser add-ons will be returned and app update will call the add-ons 
> manager to check if there are updates for the add-ons that aren't 
> compatible with the update.

Depending on the answer to the 2 above questions, if we do force the update onto the user eventually anyway, I think the Desktop browser will just check for incompatible addons anyway on its next start.

Comment 44

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #43)
> > App update calls the add-ons manager to get a list of installed add-ons 
> > and informs the user which add-ons are not compatible with the update. 
> 
> Does this happen even if the user goes to the about dialog to do the update?
> Or only on the update timer check?
Yes, it does.

> 
> Do we ever force the update for incompatible addons?
Not entirely sure what you are asking. Add-ons that are enabled, compatible with the current version, and incompatible with the update are checked. If they have an update or a compatibility info update that makes them compatible they are removed from the list of add-ons mentioned previously.

> 
> > When in the metro browser the desktop browser add-ons won't be returned 
> > and the update will continue. When in the desktop browser the desktop 
> > browser add-ons will be returned and app update will call the add-ons 
> > manager to check if there are updates for the add-ons that aren't 
> > compatible with the update.
> 
> Depending on the answer to the 2 above questions, if we do force the update
> onto the user eventually anyway, I think the Desktop browser will just check
> for incompatible addons anyway on its next start.
We don't force updates.

On next start of the desktop browser it is too late since the user has already updated while in the metro browser which doesn't know anything about the desktop browser add-ons. If the update happened in the desktop browser then the desktop browser would of course know about the desktop browser add-ons. Then using the previously mentioned conditions would inform the user that "add-on such and such" is not compatible with the update and allow the user the option of not updating.

Comment 45

[Brian R. Bondy [:bbondy]]

OK but if we download and stage the updates from the Metro browser, then run the Desktop browser to apply that staged update, I think it's too late at that point right?  So I think what you're suggesting is to only detect an update is available and then launch the Desktop browser ourselves (maybe in the background?) to do the actual update. 

I think this would pretty much invalidate the 2 big tasks I'm currently working on which is updating silently in Metro and allowing updates through the about flyout pane in Metro.

I just want to make sure I understand what you're suggesting before I bring it to Asa and ask him his opinion. Thanks!

Comment 46

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #45)
> OK but if we download and stage the updates from the Metro browser, then run
> the Desktop browser to apply that staged update, I think it's too late at
> that point right?  So I think what you're suggesting is to only detect an
> update is available and then launch the Desktop browser ourselves (maybe in
> the background?) to do the actual update. 
That isn't what I was suggesting. What I was suggesting is in metro it could switch to the desktop browser after finding an update before any of that. I am not saying it is a good solution... I am saying that doing the update in metro as currently proposed will bite some people since their add-ons won't be checked fr compatibility.

> I think this would pretty much invalidate the 2 big tasks I'm currently
> working on which is updating silently in Metro and allowing updates through
> the about flyout pane in Metro.
> 
> I just want to make sure I understand what you're suggesting before I bring
> it to Asa and ask him his opinion. Thanks!
Not a problem. I just want to make sure that Asa, etc. know about this especially since the proper place to fix this would be the add-ons manager since trying to work around this in app update in any other way than switching to the desktop browser would be beyond belief hackish and painful.

Comment 47

[Brian R. Bondy [:bbondy]]

Hi Asa,

We ran into a potentially serious snag with Metro side updates relating to add-ons on the Desktop browser side.  The main problem is that users are warned about incompatible add-ons on the Desktop side but not the Metro side before updates are applied.  Since it's the same browser executable for both browsers we can break people's add-ons in Desktop if they use the Metro browser for updates, if those add-ons are incompatible with the new version.  Adding code to check these add-ons inn the Desktop profile from the Metro browser would be non-trivial.

I think these are the options, I'd like your opinion on how to move forward:

1. Keep going like we are with doing updates in both browsers, and allow updates in the Metro about Flyout.  Accept that some users will be forced into updates that will break their add-ons.  This may be acceptable in my opinion since I think we're moving towards addon SDK anyway. (I think we're at most a week away from this)

2. Keep going like we are with doing updates in both browsers, and allow updates in the Metro about Flyout.  Spend some time adding Desktop profile add-on checking inside the Metro browser.  This may take an iteration alone. 

3. Abort supplying updates as we had planned, and only check if there is an update available, and if so just launch the desktop browser to do the update work.  This makes updates via the about flyout not possible. (+ 1 week from what #1 option would take)

4. Abort everything relating to updates for v1 and just assume that users will eventually use the Desktop browser. (0 days effort)

Comment 48

[Robert Strong (they/them - no direct email)]

I personally support option 1 even though it will upset some number of users since the real bang for the buck when it comes to resources is to get add-ons in a state where they no longer become incompatible.

Comment 49

[Asa Dotzler [:asa]]

(In reply to Brian R. Bondy [:bbondy] from comment #47)
> Hi Asa,
> 
> We ran into a potentially serious snag with Metro side updates relating to
> add-ons on the Desktop browser side.  The main problem is that users are
> warned about incompatible add-ons on the Desktop side but not the Metro side
> before updates are applied.  Since it's the same browser executable for both
> browsers we can break people's add-ons in Desktop if they use the Metro
> browser for updates, if those add-ons are incompatible with the new version.
> Adding code to check these add-ons inn the Desktop profile from the Metro
> browser would be non-trivial.
> 
> I think these are the options, I'd like your opinion on how to move forward:
> 
> 1. Keep going like we are with doing updates in both browsers, and allow
> updates in the Metro about Flyout.  Accept that some users will be forced
> into updates that will break their add-ons.  This may be acceptable in my
> opinion since I think we're moving towards addon SDK anyway. (I think we're
> at most a week away from this)

This is most appealing, IMO, but I do want to get some stats on what kind of breakage we're seeing these days with new releases. I imagine that users who are dependent on extensions in Desktop are going to be going back and forth enough that they'll get updates via the Desktop path at a reasonable rate. That will mean fewer cases of updates performed from Metro Firefox and so fewer opportunities to break add-ons silently. 

> 2. Keep going like we are with doing updates in both browsers, and allow
> updates in the Metro about Flyout.  Spend some time adding Desktop profile
> add-on checking inside the Metro browser.  This may take an iteration alone. 

Yeah. That's gonna be an awkward experience. User: Firefox, update yourself. Firefox: OK, lemme check on some things. . . Are you sure you want to update Metro Firefox because updating Metro Firefox will also update desktop Firefox and updating desktop Firefox will break some of your extensions?" That flow sure doesn't appeal to me.

> 3. Abort supplying updates as we had planned, and only check if there is an
> update available, and if so just launch the desktop browser to do the update
> work.  This makes updates via the about flyout not possible. (+ 1 week from
> what #1 option would take)

This also feels like it would be a very cumbersome UI.

> 4. Abort everything relating to updates for v1 and just assume that users
> will eventually use the Desktop browser. (0 days effort)

I worry that if a user is mostly or only using Metro, that they'll be left insecure too often for too long. 

I think we've got three cases to consider here. 

1. the user installed Firefox but only ever uses Metro Firefox.
2. the user spends time in both desktop and Metro Firefox.
3. the user only ever uses desktop Firefox. 

If all we had was 2 and 3, I'd abort further work here and just let the desktop handle things.  I worry, though, that we may eventually have a significant number of users in group 1 which doesn't open desktop Firefox nearly often enough to stay secure.

Would it be possible to do something conditionally based on whether a user is in group 1 or not? Perhaps desktop Firefox could set some "activity" bit that Metro Firefox could look at before automatically updating?

Also, is there a difference here between user-initiated updates and automatic updates?  I fear that Firefox Metro will be a long running app that is restarted rarely (ideally only on the rare Firefox crash) and so wouldn't pick up an update until the user explicitly restarted it. There may be value in some kind of "restart to update" prompt when the exe has been updated from the desktop side.

Comment 50

[Brian R. Bondy [:bbondy]]

> This is most appealing, IMO, but I do want to get some stats on what kind of 
> breakage we're seeing these days with new releases. I imagine that users who are 
> dependent on extensions in Desktop are going to be going back and forth enough 
> that they'll get updates via the Desktop path at a reasonable rate. That will
>  mean fewer cases of updates performed from Metro Firefox and so fewer
>  opportunities to break add-ons silently. 

Robert do we have telemetry data or some other type of data on this already?
If not I could add telemetry data but we wouldn't get valuable data until we let it sit through several different releases on the release channel.

Comment 51

[Robert Strong (they/them - no direct email)]

Brian, we don't from app update though collecting it from the add-ons manager has been discussed before I believe with mconnor and iirc he thought that it could be inferred via the add-ons installed which I think I was told is already collected.

Comment 52

[Brian R. Bondy [:bbondy]]

> Also, is there a difference here between user-initiated updates and automatic 
> updates?  I fear that Firefox Metro will be a long running app that is restarted
>  rarely (ideally only on the rare Firefox crash) and so wouldn't pick up an
>  update until the user explicitly restarted it. There may be value in some kind
>  of "restart to update" prompt when the exe has been updated from the 
> desktop side.

The way the patches work now, the update will be applied in the background and the next browser restart will swap out the new folder.  If Firefox Desktop starts up next it will be the one to do it.  We could maybe force an in use apply from metro though.  One of the related bugs that I recently completed allows this.

Comment 53

[Brian R. Bondy [:bbondy]]

> I think we've got three cases to consider here. 
>
> 1. the user installed Firefox but only ever uses Metro Firefox.
> 2. the user spends time in both desktop and Metro Firefox.
> 3. the user only ever uses desktop Firefox. 
>
> If all we had was 2 and 3, I'd abort further work here and just let the 
> desktop handle things.  I worry, though, that we may eventually have a 
> significant number of users in group 1 which doesn't open desktop Firefox 
> nearly often enough to stay secure.
>
> Would it be possible to do something conditionally based on whether a user is
>  in group 1 or not? Perhaps desktop Firefox could set some "activity" bit that 
> Metro Firefox could look at before automatically updating?

We could store a timestamp in the registry but I think adding more hard to test complexity and conditional logic into update logic isn't ideal.

Comment 54

[Jim Mathies [:jimm]]

(In reply to Asa Dotzler [:asa] from comment #49)
> (In reply to Brian R. Bondy [:bbondy] from comment #47)
> > 1. Keep going like we are with doing updates in both browsers, and allow
> > updates in the Metro about Flyout.  Accept that some users will be forced
> > into updates that will break their add-ons.  This may be acceptable in my
> > opinion since I think we're moving towards addon SDK anyway. (I think we're
> > at most a week away from this)
> 
> This is most appealing, IMO, but I do want to get some stats on what kind of
> breakage we're seeing these days with new releases. I imagine that users who
> are dependent on extensions in Desktop are going to be going back and forth
> enough that they'll get updates via the Desktop path at a reasonable rate.
> That will mean fewer cases of updates performed from Metro Firefox and so
> fewer opportunities to break add-ons silently. 


Can we do the update check from within metro and if there are add-on incompatibilities prompt w/a dialog giving them the option to go to desktop and finish? This is not going to happen often so doing the update in metro when we can and dumping to desktop if we need too seems like a reasonable trade off.

Comment 55

[Brian R. Bondy [:bbondy]]

The problem is we don't have the desktop profile / add-on list readily available.

Comment 56

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #55)
> The problem is we don't have the desktop profile / add-on list readily
> available.

Ah, ok, different profiles. Gotcha.

Comment 57

[Robert Strong (they/them - no direct email)]

Comment on attachment 736971 [details] [diff] [review]
Patch v2 - update js and about dialog code

Last review comment already discussed on irc with bbondy.
naming is a tad cumbersome (e.g. isResponsibleForUpdates, isResponsibleForUpdatesOfInstallation, etc.). Perhaps hasMutex would be better.

Comment 58

[Brian R. Bondy [:bbondy]]

I'm going to update it but fwiw after thinking about it a bit more I think I like the more verbose, self documenting, non-ambiguous function naming, even if it requires a bit more typing when maintaining the code. I think the understandability makes up for it.

Comment 59

[Robert Strong (they/them - no direct email)]

I would be ok with it except for the term responsible since it implies one is responsible for handling updates and the other is not when in fact it should be one is handling an update and the other is not. In other words one is responsible for handling vs. one is handling.

Comment 60

[Robert Strong (they/them - no direct email)]

This is also why I preferred the simpler naming since it requires the developer to look at the verbose documentation vs. assuming the function name says it all for the somewhat complex reality of what this does in this instance.

Comment 61

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #60)
> This is also why I preferred the simpler naming since it requires the
> developer to look at the verbose documentation vs. assuming the function
> name says it all for the somewhat complex reality of what this does in this
> instance.

In that case I'll just name it hum(); (has update mutex). Just joking :)

Comment 62

[Robert Strong (they/them - no direct email)]

Since the mutex should only be present when performing an update how about something along that is equivalent to "other instance updating"?

Comment 63

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Updater callback

Fixed review nit & stripped trailing whitespace from updatehelper.h & .cpp.
Carrying forward r+

Comment 64

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #29)
> Comment on attachment 741447 [details] [diff] [review]
> Patch v1 - Fix metro updater url
> 
> bbondy, I am a tad concerned about the other cases we use
> Services.appinfo.name... have you looked at the other cases where it is
> used? Clearing review until after both nthomas and you respond.

That seems to be the only one except for an error message format.

Comment 65

[Brian R. Bondy [:bbondy]]

> >+<!ENTITY update.notResponsible      "&brandShortName; is being updated by another instance">
> I [think] this this string is fine but you need to get UX r+ on this string.

I'll do a UX review screenshot before landing this

Comment 66

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #32)
> One thing that will bite some people with metro is that there will be no
> incompatibility check for add-ons in the non metro environment when
> performing an app update. Please bring this up to Asa, etc. It seems to me
> that it would be better to switch to native to perform an update to get
> around this issue but I will leave this to product and UX.

We're going ahead with turning the stop updates for incompatible addons pref's default from `on` to `off`.  We're also adding an enable updates through Metro checkbox which will be by default `on`.   The 2 options will be mutually exclusive.  See bug 866229 for details.

Comment 67

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #33)
> Comment on attachment 736971 [details] [diff] [review]
> Patch v2 - update js and about dialog code
> 
> >diff --git a/browser/metro/base/content/browser-ui.js b/browser/metro/base/content/browser-ui.js
> >--- a/browser/metro/base/content/browser-ui.js
> >+++ b/browser/metro/base/content/browser-ui.js
> >@@ -148,22 +148,16 @@ var BrowserUI = {
> >         PdfJs.init();
> > #ifdef MOZ_SERVICES_SYNC
> >         WeaveGlue.init();
> > #endif
> >       } catch(ex) {
> >         Util.dumpLn("Exception in delay load module:", ex.message);
> >       }
> > 
> >-#ifdef MOZ_UPDATER
> >-      // Check for updates in progress
> >-      let updatePrompt = Cc["@mozilla.org/updates/update-prompt;1"].createInstance(Ci.nsIUpdatePrompt);
> >-      updatePrompt.checkForUpdates();
> >-#endif
> >-
> What is the UX here? Is an update check performed on open?

I don't know why that code used to be there, but I don't think it should be there. The update timer should kick off updates, or the about panel from the other bug.

Comment 68

[Brian R. Bondy [:bbondy]]

Comment on attachment 741447 [details] [diff] [review]
Patch v1 - Fix metro updater url

I *think* the current plan is to do product name aliasing on the server side so I'm obsoleting this patch.

Comment 69

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - update js and about dialog code

Comment 70

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4' - Updater callback

- Rebased to m-c tip
- Commit message added

Comment 71

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3' - update js and about dialog code

- Rebased
- Added commit message

Comment 72

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - update js and about dialog code

hasUpdateMutex needed to return true on non-Windows platforms always (was returning false).

Comment 73

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5 - update js and about dialog code

Added extra real-null check for handle before using handle.isNull on Windows (was always allowing updates)

Comment 74

[Brian R. Bondy [:bbondy]]

Performed the following manual tests to verify things are working correctly:

- Verify that an update from Desktop Firefox before this patch to after this patch works w/ service
- Verify that an update from Desktop Firefox before this patch to after this patch works w/o service
- Verify that an update from Desktop Firefox before this bug to after this patch w/ service succeeds while a second instance is open.
- Verify that an update from Desktop Firefox before this bug to after this patch w/o service succeeds while a second instance is open.
- Verify that an update from Desktop Firefox after this patch to after this patch works w/ service
- Verify that an update from Desktop Firefox Desktop Firefox after this patch to after this patch works w/o service
- Verify that an update from Desktop Firefox after this patch to after this patch gives an error w/o service while a second instance is open.
- Verify that an update from Desktop Firefox after this patch to after this patch gives an error w/ service (first replace fails then falls back to normal replace which also fails with error 47) while a second instance is open.
- Verified an uninstall clears both old and new update directories
- Verified an install clears both old and new update directories
- Verified an update migrates update files to the new directory from Desktop Firefox
- Verified an update does not migrates update files if they have already been migrated
- Test limited user account updates

- Verify that an update from Metro Firefox after this patch to after this patch works w/o service
- Verify that an update from Metro Firefox after this patch to after this patch works w/ service
  ** Found that it doesn't relaunch Metro interface, posting new bug for this (We aren't handling Components.interfaces.nsIAppStartup.eAttemptQuit / NS_SUCCESS_RESTART_APP in toolkit/xre code)

- Test that manual update setting gets propagated correctly
- Test that disabled update setting gets propagated correctly
- Test that update using service setting gets propagated correctly
- Test a Desktop update while Metro is open
- Test a Metro update while Desktop is open
- Test update using XP admin
- Test update using XP limited user account
- Test Vista update using admin
- Test Vista update using limited user account

- Test that a non-background (i.e. app.update.stage.enabled=false) updates work from Desktop
- Test that a non-background (i.e. app.update.stage.enabled=false) updates work from Metro

- Check that updating in the background works

Comment 75

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/18d884c5e7df
https://hg.mozilla.org/mozilla-central/rev/c1b755912e76