967403 - Opening metrofx with a pending update will launch/close the browser and leave an entry under "App List"

Status: RESOLVED INCOMPLETE

(Firefox for Metro Graveyard :: Install/Update, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

When you're updating the Firefox Metro browser via the "About" flyout, selecting "Restart Browser" and then quickly taping on the "Nightly" tile will quickly start launching the browser (the splash screen appears) and then it Firefox Metro browser disappears and you're return to the Windows Metro screen.

This sounds like the same issue described in Bug #950241 comment #0, but a small corner case compared to the original issue.

Example of issue:
- http://youtu.be/Yqjs4uJWlVk

Steps to reproduce the issue:

1) Download and install an older version of Nightly (2-3 days old)
2) Launch Firefox Metro and slide in the Windows Charm, select "Settings -> About"
3) The new update will start downloading and then update the browser
4) Tap on "Restart to Update"
5) The browser will disappear, quickly tap on the "Nightly" tile on the Windows Metro screen. You'll notice that the browser will quickly appear/disappear giving the illusion of a crash.

Current Behavior:

- Quickly taping on the Nightly tile after the browser disappears will relaunch and then dismiss the browser making it seem like it crashed. After it disappears, you'll notice that its listed under the "App List"

Expected Behavior:

- While the browser is being updated, the user shouldn't be able to launch another instance until the browser has been updated.

Found the issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-01-03-02-04-mozilla-central/
- Once the above build was installed, updated to the latest version via "About" flyout

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Another easier way to reproduce the same issue:

1) Install an older version of Firefox Metro (2-3 days older)
2) Once installed, launch Firefox Metro and slide in the Windows Charm
3) Select "Settings -> About" (the update should start right away)
4) Wait till the update is completed and the "Restart Browser" button appears
5) Now rather than taping the button, close Firefox Metro by sliding it to the bottom (close it correctly)

You'll run into the same problem when you tap/click the Firefox Metro tile after step #5

Comment 2

[Jim Mathies [:jimm]]

Yeah as you mentioned in comment 1 this is a corner case dupe of bug 950241.

If the CEH could detect pending updates, it could launch the desktop to complete the update first. bbondy, is this possible / sounds too crazy?

Comment 3

[Jim Mathies [:jimm]]

I don't think we can classify this as "corner case" either since generally we'll do updates in the background. When the user closes and restarts, they get bug 950241.

Comment 4

[Jim Mathies [:jimm]]

http://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp#237

Maybe we can try to passively read the status.update file, although I'm a little wary of that. Also not sure what directory we should look in for it.

Comment 5

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #2)
> Yeah as you mentioned in comment 1 this is a corner case dupe of bug 950241.
> 
> If the CEH could detect pending updates, it could launch the desktop to
> complete the update first. bbondy, is this possible / sounds too crazy?

Instead of trying to touch the update status file, I'd first see if this behavior can be reproduced when you lock the firefox.exe exe.  I it can, then it sounds like we can just try to open the exe for read access before attempting to launch it.  If we don't have read access, don't try to launch it.

Comment 6

[Jim Mathies [:jimm]]

bbondy and I discussed this on irc. In cases where the user doesn't restart via the about flyout we're going to flag the ceh from nsUpdateDriver to restart silently in desktop once when an update is pending or applied in the background.

Comment 7

[Jim Mathies [:jimm]]

I think rather than mess with update code, I'll do this up in front end. I'm most concerned about updates that have been downloaded in the background while the browser is running.

Comment 8

[Jim Mathies [:jimm]]

There's another corner case here that I'm not sure how to address: a downloaded update within desktop that's ready to install, and the user switches to metro.

Comment 9

[Marco Mucci [:MarcoM]]

Hey Jim, can you provide a point value.

Comment 10

[Jim Mathies [:jimm]]

This bug may be open for a bit. I'll be throwing patches at oak to generate updates for testing.

Comment 11

[Marco Mucci [:MarcoM]]

Thanks for letting me know that it will be open for a bit.  I'll remove it from this iteration and we can re-evaluate at the time it is ready to land as to which iteration it should be included in.

Comment 12

[Jim Mathies [:jimm]]

Attachment: wip

Comment 13

[Jim Mathies [:jimm]]

Attachment: wip v2

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 15

[Jim Mathies [:jimm]]

Attachment: part 1 - tag staged updates via the registry

Comment 16

[Jim Mathies [:jimm]]

Attachment: part 2 - process staged updates on shutdown

Comment 17

[Jim Mathies [:jimm]]

Attachment: part 3 - handle background update processing in xre for metro

Comment 18

[Jim Mathies [:jimm]]

Attachment: part 4 - misc. cleanup in ceh

Comment 19

[Jim Mathies [:jimm]]

What I'm doing here is firing off a background desktop process using a command line that's already available for background update processing when the user shuts down using the close gesture and an update is currently staged.

This works around the one remaining common path to the situation where we get the double launch when tapping the tile. It doesn't solve the problem 100%, afaict we don't get called back on 8.1 when you close via the app thumbnails or when the browser gets killed via the task manager. So there's still a chance users will run into the double launch weirdness, but its a corner case we can't work around.

Comment 20

[Jim Mathies [:jimm]]

Try push - 
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=468728287e25

also all of this code is on oak, you can trigger an update between the last two nightlies to test.

Comment 21

[Brian R. Bondy [:bbondy]]

Comment on attachment 8377527 [details] [diff] [review]
part 2 - process staged updates on shutdown

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

BTW I'd like rstrong's thoughts on launching process-updates directly too, please flag him on the next patch or ask him in a separate conversation

::: widget/windows/WinUtils.cpp
@@ +333,5 @@
> +  LONG result =
> +    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | KEY_WOW64_32KEY, &key);
> +  if (result != ERROR_SUCCESS) {
> +    result =
> +      ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | KEY_WOW64_64KEY, &key);

I don't think this first check 32-bit and then check 64-bit is correct.
It would mean that a 64-bit build would first try to check a 32-bit key.  But I think both builds should be treated differently. I think there should be no such flags here and only one check.

@@ +364,5 @@
> +  LONG result =
> +    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_WRITE | KEY_WOW64_32KEY, &key);
> +  if (result != ERROR_SUCCESS) {
> +    result =
> +      ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_WRITE | KEY_WOW64_64KEY, &key);

Ditto 32/64 bit registry handling here.

@@ +370,5 @@
> +      return false;
> +    }
> +  }
> +
> +  result = RegDeleteValueW(key, aValueName);

There's a handle leak here, we need to close key

@@ +374,5 @@
> +  result = RegDeleteValueW(key, aValueName);
> +  if (result != ERROR_SUCCESS) {
> +    return false;
> +  }
> +  return true;

nit: return result == ERROR_SUCCESS;

::: widget/windows/winrt/MetroAppShell.cpp
@@ +329,5 @@
> +  if (!GetModuleFileNameW(NULL, path, MAX_PATH)) {
> +    WinUtils::Log("Couldn't find process path. %d", GetLastError());
> +    return;
> +  }
> +  

nit: cleanup trailing whitespace here and above.

@@ +338,5 @@
> +  startInfo.cb = sizeof(STARTUPINFO);
> +  startInfo.wShowWindow = SW_SHOWNORMAL;
> +
> +  nsAutoString commandLine;
> +  commandLine.Append(path);

Does this path need to be quoted?

Comment 22

[Brian R. Bondy [:bbondy]]

Comment on attachment 8377526 [details] [diff] [review]
part 1 - tag staged updates via the registry

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

::: browser/metro/base/content/flyoutpanels/AboutFlyoutPanel.js
@@ +52,5 @@
>  #endif
>    },
>  
> +  /*
> +   * Connect an update ovserver that creates a registry key indicating

nit; ovserver

@@ +71,5 @@
> +                       "SOFTWARE\\Mozilla\\Firefox",
> +                       wrk.ACCESS_WRITE);
> +            wrk.writeIntValue(kStagedUpdateKey, 1);
> +          } catch (ev) {
> +            Services.console.logStringMessage("exception:" + ev);

nit: "exception: "

Comment 23

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #21)
> Comment on attachment 8377527 [details] [diff] [review]
> part 2 - process staged updates on shutdown
> 
> Review of attachment 8377527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> BTW I'd like rstrong's thoughts on launching process-updates directly too,
> please flag him on the next patch or ask him in a separate conversation

np.

> ::: widget/windows/WinUtils.cpp
> @@ +333,5 @@
> > +  LONG result =
> > +    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | KEY_WOW64_32KEY, &key);
> > +  if (result != ERROR_SUCCESS) {
> > +    result =
> > +      ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | KEY_WOW64_64KEY, &key);
> 
> I don't think this first check 32-bit and then check 64-bit is correct.
> It would mean that a 64-bit build would first try to check a 32-bit key. 
> But I think both builds should be treated differently. I think there should
> be no such flags here and only one check.

This is duplicated from our existing reg open utils, which were formulated from registry functions we've had around for a while. I understand the concern, but I'm a little wary of changing the behavior in this bug. How about I land this as is and file a follow up cc'ing people in to discuss?

http://mxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.cpp#268

> 
> @@ +364,5 @@
> > +  LONG result =
> > +    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_WRITE | KEY_WOW64_32KEY, &key);
> > +  if (result != ERROR_SUCCESS) {
> > +    result =
> > +      ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_WRITE | KEY_WOW64_64KEY, &key);
> 
> Ditto 32/64 bit registry handling here.
> 
> @@ +370,5 @@
> > +      return false;
> > +    }
> > +  }
> > +
> > +  result = RegDeleteValueW(key, aValueName);
> 
> There's a handle leak here, we need to close key

oops. fixed.

> 
> @@ +374,5 @@
> > +  result = RegDeleteValueW(key, aValueName);
> > +  if (result != ERROR_SUCCESS) {
> > +    return false;
> > +  }
> > +  return true;
> 
> nit: return result == ERROR_SUCCESS;
> 
> ::: widget/windows/winrt/MetroAppShell.cpp
> @@ +329,5 @@
> > +  if (!GetModuleFileNameW(NULL, path, MAX_PATH)) {
> > +    WinUtils::Log("Couldn't find process path. %d", GetLastError());
> > +    return;
> > +  }
> > +  
> 
> nit: cleanup trailing whitespace here and above.
> 
> @@ +338,5 @@
> > +  startInfo.cb = sizeof(STARTUPINFO);
> > +  startInfo.wShowWindow = SW_SHOWNORMAL;
> > +
> > +  nsAutoString commandLine;
> > +  commandLine.Append(path);
> 
> Does this path need to be quoted?

Yes, good catch it probably should be.

Comment 24

[Brian R. Bondy [:bbondy]]

> This is duplicated from our existing reg open utils, which were formulated from registry functions we've
> had around for a while. I understand the concern, but I'm a little wary of changing the behavior in this 
> bug. How about I land this as is and file a follow up cc'ing people in to discuss?

But I think it is wrong in this case. 
I'd be more OK with it if the JavaScript patch did the same thing and checked the same location, but currently it doesn't.  This will break the x64 build handling, which may become a supported thing sooner than later.  I don't think it should land like this, maybe we can pass a flag for using the 2 different reg locations, but just not use that flag right now.

Comment 25

[Jim Mathies [:jimm]]

Attachment: part 2 - process staged updates on shutdown

masayuki, will this suffice for these mouse driver checks or should we make calls for both 64-bit and 32-bit hives regardless of the bitness of our app? Do these drivers write to the hive that matches the machine bitness?

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 8382949 [details] [diff] [review]
part 2 - process staged updates on shutdown

rstrong, curious does the 64-bit installer write to the 64-bit registry hive? If so this change shouldn't impact task bar association. If for some reason the 64-bit installer writes to the 32-bit hive, this will need to be updated.

Specifically I'm curious about the InitHashAppModelId routine - 

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#5214

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8382949 [details] [diff] [review]
part 2 - process staged updates on shutdown

(In reply to Jim Mathies [:jimm] from comment #25)
> masayuki, will this suffice for these mouse driver checks or should we make
> calls for both 64-bit and 32-bit hives regardless of the bitness of our app?
> Do these drivers write to the hive that matches the machine bitness?

Although, I'm not familiar with registry access with WOW64, it might fail to access the driver's registry from 32bit process on 64bit environment.

As I checked the registry quickly on my 2 machines, both Logitech and Synaptics are registered not under Wow6432Node. If 32bit process cannot access outside Wow6432Node without KEY_WOW64_64KEY, this may cause regressions. Otherwise, there _might_ not be problem.

I'm not sure if no mouse drivers use 32bit key because their utility apps may be 32bit process. So, I feel that this change is a little bit risky since we cannot test all environments which are installed the devices. In other words, we cannot test the result before the release, perhaps.

And I'd like you to define a constant for 0 of aExtendedAccess. It's not clear when I see the callers.

They must not be called two or more times, if I remember correctly. So, I recommend that you should keep accessing both entries. I believe that we can ignore the running cost.

Comment 28

[Jim Mathies [:jimm]]

due to the changes here don't think this is safe to uplift to beta. we'll just let it roll out with the trains.

Comment 29

[Jim Mathies [:jimm]]

masayuki, any idea why this check doesn't return true?

http://hg.mozilla.org//mozilla-central/annotate/a98a1d78817f/widget/windows/WinMouseScrollHandler.cpp#l1345

Looks like a bug in the original landing maybe?

https://bugzilla.mozilla.org/attachment.cgi?id=599532&action=diff

Comment 30

[Jim Mathies [:jimm]]

Attachment: part 2 updated

Comment 31

[Jim Mathies [:jimm]]

Attachment: part 2 - process staged updates on shutdown

Comment 32

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

(In reply to Jim Mathies [:jimm] from comment #26)
> Comment on attachment 8382949 [details] [diff] [review]
> part 2 - process staged updates on shutdown
> 
> rstrong, curious does the 64-bit installer write to the 64-bit registry
> hive? If so this change shouldn't impact task bar association. If for some
> reason the 64-bit installer writes to the 32-bit hive, this will need to be
> updated.
It does
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#5018

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jim Mathies [:jimm] from comment #29)
> masayuki, any idea why this check doesn't return true?
> 
> http://hg.mozilla.org//mozilla-central/annotate/a98a1d78817f/widget/windows/
> WinMouseScrollHandler.cpp#l1345
> 
> Looks like a bug in the original landing maybe?
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=599532&action=diff

Yeah, it must be so.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8383180 [details] [diff] [review]
part 2 - process staged updates on shutdown

>+  const static char16ptr_t mainOpt = L"Software\\Elantech\\MainOption";
>   bool foundKey =
>     WinUtils::GetRegistryKey(HKEY_CURRENT_USER,
>-                             L"Software\\Elantech\\MainOption",
>-                             L"DriverVersion",
>+                             mainOpt, L"DriverVersion",
>+                             WinUtils::Hive32Bit,
>+                             buf, sizeof buf) ||
>+    WinUtils::GetRegistryKey(HKEY_CURRENT_USER,
>+                             mainOpt, L"DriverVersion",
>+                             WinUtils::Hive64Bit,
>                              buf, sizeof buf);

Calling twice in a lot of places looks redundant. How about additional value like Hive32BitAnd64Bit? Then, WinUtils can call twice with Hive32Bit and Hive64Bit internally.

>+  const static char16ptr_t alpsTrackPoint = L"Software\\Alps\\Apoint\\TrackPoint";
>   if (WinUtils::HasRegistryKey(HKEY_CURRENT_USER,
>-                               L"Software\\Alps\\Apoint\\TrackPoint")) {
>+                               alpsTrackPoint,
>+                               WinUtils::Hive32Bit) ||
>+      WinUtils::HasRegistryKey(HKEY_CURRENT_USER,
>+                               alpsTrackPoint,
>+                               WinUtils::Hive64Bit)) {
>     PR_LOG(gMouseScrollLog, PR_LOG_ALWAYS,
>       ("MouseScroll::Device::TrackPoint::IsDriverInstalled(): "
>        "Alps's TrackPoint driver is found"));
>+    return true;
>   }

Thank you!!

>+  /**
>+   * Extended access registry hive target ids for 64 and 32 bit registry node
>+   * selection. These are not masks, pass a single value only.
>+   * HiveAppDefault - Access the hive identical to the bitness of the running
>+   *   application.
>+   * Hive64Bit - Access a 64-bit hive from either a 32-bit or 64-bit app.
>+   * Hive32Bit - Access a 32-bit hive from either a 32-bit or 64-bit app.
>+   */
>+  enum HiveTarget {
>+    HiveAppDefault = 0,
>+    Hive64Bit = 1,
>+    Hive32Bit = 2
>+  }; 

This is great approach! Additionall, as I said above, adding Hive32BitAnd64Bit is better.

nit: The "{" should be next line under "e" of enum since this is not control structure, same as class or struct definition.

Comment 35

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
>
> Calling twice in a lot of places looks redundant. How about additional value
> like Hive32BitAnd64Bit? Then, WinUtils can call twice with Hive32Bit and
> Hive64Bit internally.
> 

Great idea. Will add this and repost.

Comment 36

[Jim Mathies [:jimm]]

Attachment: part 2 - process staged updates on shutdown

FWIW I tried putting together some cpp unit tests for these, but ran into all sorts of problem compiling and linking external/internal string apis. I'm going to file a follow up on trying to get those working.

Comment 37

[Jim Mathies [:jimm]]

Attachment: part 2 - process staged updates on shutdown

- plus the nsTextStore use.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8383754 [details] [diff] [review]
part 2 - process staged updates on shutdown

> /* static */
> int32_t
> MouseScrollHandler::Device::Elantech::GetDriverMajorVersion()
> {
>   wchar_t buf[40];
>   // The driver version is found in one of these two registry keys.
>+  const static char16ptr_t mainOpt = L"Software\\Elantech\\MainOption";

kMainOpt?

>+  const static char16ptr_t elantech = L"Software\\Elantech";

kElantech?

>@@ -1329,76 +1332,83 @@
>  * Device::TrackPoint
>  *
>  ******************************************************************************/
> 
> /* static */
> bool
> MouseScrollHandler::Device::TrackPoint::IsDriverInstalled()
> {
>-  if (WinUtils::HasRegistryKey(HKEY_CURRENT_USER,
>-                               L"Software\\Lenovo\\TrackPoint")) {
>+  const static char16ptr_t trackPoint = L"Software\\Lenovo\\TrackPoint";


kTrackPoint?

>+  const static char16ptr_t alpsTrackPoint = L"Software\\Alps\\Apoint\\TrackPoint";

kAlpsTrackPoint?

> /******************************************************************************
>  *
>  * Device::UltraNav
>  *
>  ******************************************************************************/
> 
> /* static */
> bool
> MouseScrollHandler::Device::UltraNav::IsObsoleteDriverInstalled()
> {
>+  const static char16ptr_t ultraNav = L"Software\\Lenovo\\UltraNav";

kUltraNav?

>+  const static char16ptr_t ultraNavUSB = L"Software\\Synaptics\\SynTPEnh\\UltraNavUSB";

kUltraNavUSB?

>+  const static char16ptr_t ultraNavPS2 = L"Software\\Synaptics\\SynTPEnh\\UltraNavPS2";

kUltraNavPS2?

>+  const static char16ptr_t synTP = L"Software\\Synaptics\\SynTP\\Install";

kSynTP?

>diff -r dc83967b4d6b widget/windows/WinTaskbar.cpp
>--- a/widget/windows/WinTaskbar.cpp	Fri Feb 28 07:39:59 2014 -0600
>+++ b/widget/windows/WinTaskbar.cpp	Fri Feb 28 10:47:51 2014 -0600

I'm not sure this part. If this original author is not you, please request additional review to somebody.

>diff -r dc83967b4d6b widget/windows/WinUtils.cpp
>--- a/widget/windows/WinUtils.cpp	Fri Feb 28 07:39:59 2014 -0600
>+++ b/widget/windows/WinUtils.cpp	Fri Feb 28 10:47:51 2014 -0600
>+static bool
>+GetRegistryKeyInt(HKEY aRoot,

GetRegistryKeyStr()?

>+                  char16ptr_t aKeyName,
>+                  char16ptr_t aValueName,
>+                  ACCESS_MASK aHive,

REGSAM?
http://msdn.microsoft.com/ja-jp/library/ms724897%28v=vs.85%29.aspx

>+                  wchar_t* aBuffer,
>+                  DWORD aBufferLength)
> {
>-  NS_PRECONDITION(aKeyName, "The key name is NULL");
>-
>   HKEY key;
>   LONG result =
>-    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | KEY_WOW64_32KEY, &key);
>+    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ |
>+                    aHive, &key);

Cannot be these in one line?
I.e., |   ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | aHive, &key);|.

>+WinUtils::GetRegistryKey(HKEY aRoot,
>+                         char16ptr_t aKeyName,
>+                         char16ptr_t aValueName,
>+                         HiveTarget aExtendedAccess,
>+                         wchar_t* aBuffer,
>+                         DWORD aBufferLength)
>+{
>+  NS_PRECONDITION(aKeyName, "The key name is NULL");
>+  switch (aExtendedAccess) {
>+    case HiveAppDefault:
>+      return GetRegistryKeyInt(aRoot, aKeyName, aValueName, 0,
>+                               aBuffer, aBufferLength);
>+      break;

Please remove the unnecessary break.

>+    case Hive64Bit:
>+      return GetRegistryKeyInt(aRoot, aKeyName, aValueName, KEY_WOW64_64KEY,
>+                               aBuffer, aBufferLength);
>+      break;

Here too.

>+    case Hive32Bit:
>+      return GetRegistryKeyInt(aRoot, aKeyName, aValueName, KEY_WOW64_32KEY,
>+                               aBuffer, aBufferLength);
>+      break;

Here too.

>+    case Hive32BitThen64Bit:
>+      if (GetRegistryKeyInt(aRoot, aKeyName, aValueName, KEY_WOW64_32KEY,
>+          aBuffer, aBufferLength)) {
>+        return true;
>+      }
>+      return GetRegistryKeyInt(aRoot, aKeyName, aValueName, KEY_WOW64_64KEY,
>+                               aBuffer, aBufferLength);
>+      break;

Here too.

>+    case HiveAppDefaultThenOther:
>+#if defined(_WIN64)
>+      ACCESS_MASK firstPass = KEY_WOW64_64KEY;
>+      ACCESS_MASK seconPass = KEY_WOW64_32KEY;
>+#else
>+      ACCESS_MASK firstPass = KEY_WOW64_32KEY;
>+      ACCESS_MASK seconPass = KEY_WOW64_64KEY;
>+#endif

Use REGSAM if you change the argument's type of GetRegistryKeyInt().

>+      if (GetRegistryKeyInt(aRoot, aKeyName, aValueName, firstPass,
>+                            aBuffer, aBufferLength)) {
>+        return true;
>+      }
>+      return GetRegistryKeyInt(aRoot, aKeyName, aValueName, seconPass,
>+                               aBuffer, aBufferLength);
>+      break;

Remove this break.

>+  }
>+  PR_NOT_REACHED("invalid parameter to GetRegistryKey");

MOZ_CRASH()? I think PR_NOT_REACHED() isn't used in new patches.

>+static bool
>+GetRegistryKeyInt(HKEY aRoot,
>+                  char16ptr_t aKeyName,
>+                  char16ptr_t aValueName,
>+                  ACCESS_MASK aHive,

Use REGSAM if you agree above.

>+                  DWORD* aValue)
>+{
>+  HKEY key;
>+  LONG result =
>+    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ |
>+                    aHive, &key);

Cannot these in one line?

>+  if (result != ERROR_SUCCESS) {
>+    return false;
>+  }
>+
>+  DWORD type;
>+  DWORD bufferSize = sizeof(DWORD);
>+  result =
>+    ::RegQueryValueExW(key, aValueName, nullptr, &type, (BYTE*) aValue,
>+                       &bufferSize);
>+  ::RegCloseKey(key);
>+  if (result != ERROR_SUCCESS || type != REG_SZ) {

Is this right? REG_DWORD?

>+#if defined(_WIN64)
>+      ACCESS_MASK firstPass = KEY_WOW64_64KEY;
>+      ACCESS_MASK seconPass = KEY_WOW64_32KEY;
>+#else
>+      ACCESS_MASK firstPass = KEY_WOW64_32KEY;
>+      ACCESS_MASK seconPass = KEY_WOW64_64KEY;
>+#endif

Use same type as the argument type if you'll change it.

>+      if (GetRegistryKeyInt(aRoot, aKeyName, aValueName, firstPass, aValue)) {
>+        return true;
>+      }
>+      return GetRegistryKeyInt(aRoot, aKeyName, aValueName, seconPass, aValue);
>+  }
>+  NS_NOTREACHED("invalid parameter to GetRegistryKey");

MOZ_CRASH(), perhaps.

>+static bool
>+DeleteRegistryValueInt(HKEY aRoot,

"Int" is necessary?

>+                       char16ptr_t aKeyName,
>+                       char16ptr_t aValueName,
>+                       ACCESS_MASK aHive)

Check the type. Perhaps, REGSAM.

>+{
>+  HKEY key;
>+  LONG result =
>+    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_WRITE |
>+                    aHive, &key);

I guess that this can be one line.

>+/* static */
>+bool
>+WinUtils::DeleteRegistryValue(HKEY aRoot,
>+                              char16ptr_t aKeyName,
>+                              char16ptr_t aValueName,
>+                              HiveTarget aExtendedAccess)
>+{

>+  NS_NOTREACHED("invalid parameter to DeleteRegistryValue");

MOZ_CRASH()?

>+static bool
>+HasRegistryKeyInt(HKEY aRoot, char16ptr_t aKeyName,
>+                  ACCESS_MASK aHive)

I don't understand why this has "Int" postfix too.

And check the argument type like above.

>+{
>+  HKEY key;
>+  LONG result =
>+    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ |
>+                    aHive, &key);

Make these one line.

>+/* static */
>+bool
>+WinUtils::HasRegistryKey(HKEY aRoot, char16ptr_t aKeyName,
>+                         HiveTarget aExtendedAccess)
> {
>   MOZ_ASSERT(aRoot, "aRoot must not be NULL");
>   MOZ_ASSERT(aKeyName, "aKeyName must not be NULL");
>-  HKEY key;
>-  LONG result =
>-    ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | KEY_WOW64_32KEY, &key);
>-  if (result != ERROR_SUCCESS) {
>-    result =
>-      ::RegOpenKeyExW(aRoot, aKeyName, 0, KEY_READ | KEY_WOW64_64KEY, &key);
>-    if (result != ERROR_SUCCESS) {
>-      return false;
>-    }
>+  switch (aExtendedAccess) {
>+    case HiveAppDefault:
>+      return HasRegistryKeyInt(aRoot, aKeyName, 0);
>+    case Hive64Bit:
>+      return HasRegistryKeyInt(aRoot, aKeyName, KEY_WOW64_64KEY);
>+    case Hive32Bit:
>+      return HasRegistryKeyInt(aRoot, aKeyName, KEY_WOW64_32KEY);
>+    case HiveAppDefaultThenOther:
>+    case Hive32BitThen64Bit:
>+      return HasRegistryKeyInt(aRoot, aKeyName, KEY_WOW64_32KEY) |
>+        HasRegistryKeyInt(aRoot, aKeyName, KEY_WOW64_64KEY);

"||" ?

>   }
>-  ::RegCloseKey(key);
>-  return true;
>+  NS_NOTREACHED("invalid parameter to HasRegistryKey");

MOZ_CRASH().

>+  enum HiveTarget
>+  {
>+    HiveAppDefault = 0,
>+    Hive64Bit = 1,
>+    Hive32Bit = 2,
>+    Hive32BitThen64Bit = 3,
>+    HiveAppDefaultThenOther = 4
>+  }; 

I think that you can omit the values. And remove the whitespace of the after ";".

>diff -r dc83967b4d6b widget/windows/winrt/MetroApp.cpp
>--- a/widget/windows/winrt/MetroApp.cpp	Fri Feb 28 07:39:59 2014 -0600
>+++ b/widget/windows/winrt/MetroApp.cpp	Fri Feb 28 10:47:51 2014 -0600

I'm not sure what are you doing in metro part. Please ask additional review to somebody.

Let me check new patch, again.

Comment 39

[Jim Mathies [:jimm]]

> >diff -r dc83967b4d6b widget/windows/WinTaskbar.cpp
> >--- a/widget/windows/WinTaskbar.cpp	Fri Feb 28 07:39:59 2014 -0600
> >+++ b/widget/windows/WinTaskbar.cpp	Fri Feb 28 10:47:51 2014 -0600
> 
> I'm not sure this part. If this original author is not you, please request
> additional review to somebody.

bbondy already gave this part r+.

> >diff -r dc83967b4d6b widget/windows/WinUtils.cpp
> >--- a/widget/windows/WinUtils.cpp	Fri Feb 28 07:39:59 2014 -0600
> >+++ b/widget/windows/WinUtils.cpp	Fri Feb 28 10:47:51 2014 -0600
> >+static bool
> >+GetRegistryKeyInt(HKEY aRoot,
> 
> GetRegistryKeyStr()?


Actually what I wanted was something to indicate "internal". I guess "Int" isn't the best thing to use here, I'll come up with something better. 

> 
> >+                  char16ptr_t aKeyName,
> >+                  char16ptr_t aValueName,
> >+                  ACCESS_MASK aHive,
> 
> REGSAM?
> http://msdn.microsoft.com/ja-jp/library/ms724897%28v=vs.85%29.aspx

Either will do, REGSAM is a typed ACCESS_MASK which is a DWORD.

> >+  }
> >+  PR_NOT_REACHED("invalid parameter to GetRegistryKey");
> 
> MOZ_CRASH()? I think PR_NOT_REACHED() isn't used in new patches.

Not a fan of crashing for something like this, an assert and a failure seems safer. But I'll update them all the same.

> >+  enum HiveTarget
> >+  {
> >+    HiveAppDefault = 0,
> >+    Hive64Bit = 1,
> >+    Hive32Bit = 2,
> >+    Hive32BitThen64Bit = 3,
> >+    HiveAppDefaultThenOther = 4
> >+  }; 
> 
> I think that you can omit the values. And remove the whitespace of the after
> ";".

Why remove the number?

Comment 40

[Jim Mathies [:jimm]]

> >+  if (result != ERROR_SUCCESS) {
> >+    return false;
> >+  }
> >+
> >+  DWORD type;
> >+  DWORD bufferSize = sizeof(DWORD);
> >+  result =
> >+    ::RegQueryValueExW(key, aValueName, nullptr, &type, (BYTE*) aValue,
> >+                       &bufferSize);
> >+  ::RegCloseKey(key);
> >+  if (result != ERROR_SUCCESS || type != REG_SZ) {
> 
> Is this right? REG_DWORD?

Yep, my mistake - copy paste error.

> >+      return HasRegistryKeyInt(aRoot, aKeyName, KEY_WOW64_32KEY) |
> >+        HasRegistryKeyInt(aRoot, aKeyName, KEY_WOW64_64KEY);
> 
> "||" ?

I wanted | so that both get called. The header comments state both will be deleted if they exist:

* Hive32BitThen64Bit - search 32 bit hive first, if that fails search 64bit
*   hive. For delete operations this means 'delete both' regardless of which
*   exists. For 'has key' operations returns true if the key exists in
*   either hive.
* HiveAppDefaultThenOther - search app default hive first, if that fails
*   search the other. For delete and 'has key' operations this has identical
*   behavior to Hive32BitThen64Bit.

Comment 41

[Jim Mathies [:jimm]]

Attachment: part 2 - process staged updates on shutdown

Comment 42

[Jim Mathies [:jimm]]

Attachment: part 2 - process staged updates on shutdown

I think this covers everything except in cases where I disagreed with your review nit. Please let me know what you think.

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8385265 [details] [diff] [review]
part 2 - process staged updates on shutdown

r=masayuki except WinTaskbar.cpp, MetroApp.cpp, MetroAppShell.cpp and MetroAppShell.h.

Could you add a comment to explain the "|" in DeleteRegistryValue() is intentional code? It look tricky to me. And perhaps, somebody may be confused.

Comment 44

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #43)
> Comment on attachment 8385265 [details] [diff] [review]
> part 2 - process staged updates on shutdown
> 
> r=masayuki except WinTaskbar.cpp, MetroApp.cpp, MetroAppShell.cpp and
> MetroAppShell.h.
> 
> Could you add a comment to explain the "|" in DeleteRegistryValue() is
> intentional code? It look tricky to me. And perhaps, somebody may be
> confused.

sure, thanks for the reviews.

Comment 45

[Brian R. Bondy [:bbondy]]

Comment on attachment 8385265 [details] [diff] [review]
part 2 - process staged updates on shutdown

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

::: widget/windows/WinTaskbar.cpp
@@ +307,5 @@
>                                   sizeof buf)) {
>        aDefaultGroupId.Assign(buf);
>      } else if (WinUtils::GetRegistryKey(HKEY_CURRENT_USER,
>                                          regKey.get(),
> +                                        path, WinUtils::HiveAppDefault,

nit: seems like each param is on a diff line, except for this line. 
Maybe combine other lines as per 80 char line limit rule.

::: widget/windows/WinUtils.cpp
@@ +452,5 @@
> +                                         KEY_WOW64_32KEY);
> +    case HiveAppDefaultThenOther:
> +    case Hive32BitThen64Bit:
> +      return DeleteRegistryValueInternal(aRoot, aKeyName, aValueName,
> +                                         KEY_WOW64_32KEY) |

Maybe add a comment just to clarify your intention.
Just to make sure, you intended a bitwise operator here right? You want both to execute right (as per .h doc)?

::: widget/windows/WinUtils.h
@@ +170,5 @@
> +   * @param aKeyName The name of the registry key to open.
> +   * @param aValueName The name of the registry value in the specified key whose
> +   *   value is to be deleted.
> +   * @param aExtendedAccess hive target when deleting the key.
> +   * @return Whether the value exists and was deleted.

nit: If hive is Hive32BitThen64Bit or HiveAppDefaultThenOther returns true when delete from either hive, or both.

::: widget/windows/winrt/MetroAppShell.cpp
@@ +335,5 @@
> +  commandLine.Append(path);
> +  commandLine.Append(L"\"");
> +  commandLine.Append(L" --process-updates");
> +  wchar_t* paramBuf = new wchar_t[commandLine.Length() + 1];
> +  wcscpy(paramBuf, commandLine.get());

Can't we just use commandLine.BeginWriting()?

Comment 46

[Jim Mathies [:jimm]]

> Maybe add a comment just to clarify your intention.
> Just to make sure, you intended a bitwise operator here right? You want both
> to execute right (as per .h doc)?

masayuki asked for the same, I've updated my local patch.

> > +  wchar_t* paramBuf = new wchar_t[commandLine.Length() + 1];
> > +  wcscpy(paramBuf, commandLine.get());
> 
> Can't we just use commandLine.BeginWriting()?

I'm not sure, I'll look. I copied this over since the docs say windows might write over the string, including setting early nulls.

Comment 47

[Brian R. Bondy [:bbondy]]

Yep I'm familiar with the doc but BeginWriting already returns a writable buffer, so I think it should work in this case.

Comment 48

[Jim Mathies [:jimm]]

To retest all this after all the review changes I'd like to do some updating. Can I use oak for a few days?

Comment 49

[Brian R. Bondy [:bbondy]]

I need it today because I'm in the process of testing something that should land today or tomorrow, but you can use it tomorrow. Is that OK?

Comment 50

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #49)
> I need it today because I'm in the process of testing something that should
> land today or tomorrow, but you can use it tomorrow. Is that OK?

Sure, sounds good. I also want to look at bug 967394 while I'm at it.

Comment 51

[Firefox Bug Husbandry Bot]

Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354