950241 - Takes two tries to start up Firefox in Touch mode

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Karen Rudnitski [:kar]]

On my Surface Pro 2, Win 8.1. I have no open apps.

I tap on the 'nightly' icon and the Firefox nightly splash screen twirls in front of me, then disappears and it goes back to the main Touch Windows screen. If I see whether there are open apps, it's listed. But I have to tap on on it to get Firefox Touch launched (or tap on the nightly icon again).

I JUST updated to the latest nightly, and it was also doing this on the 28 branch.

Comment 1

[Karen Rudnitski [:kar]]

(don't know how this landed as a security-sensitive bug - that was certainly not my intention, nor am I sure how that happened!)

Comment 2

[Karen Rudnitski [:kar]]

Therefore please uncheck this bug if you could so the metro team can figure out what's happening in this bug :)

Comment 3

[Matt Brubeck (:mbrubeck)]

I also see this frequently on my Surface Pro 2, though I can't reproduce it 100% of the time.  I have also seen this occasionally with other Metro apps, though I think it may happen more with Nightly than with other apps.

Comment 4

[Jim Mathies [:jimm]]

I see this fairly often when there's an update pending.

Comment 5

[Karen Rudnitski [:kar]]

it's been happening every time on my SP2 for the past week, except yesterday. Yesterday it started on the first tap.

Comment 6

[Jim Mathies [:jimm]]

Attachment: debug patch

Lets start by getting rid of this option. According to msdn, you have to have debug mode set on the app for this to work. Forcing a restart via the about panel in my local build always fails with this set. Might be something they changed in 8.1.

Comment 7

[Jim Mathies [:jimm]]

Attachment: remove no splash option

oops, wrong patch

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 8348662 [details] [diff] [review]
remove no splash option

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

Ya I don't remember reading anything about that debug mode stuff and I've read the documentation before.  Maybe something new.

Comment 9

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/7f5234c69f58

Me too, I remember looking at those docs when that landed. Didn't see anything about debug mode. They must have forgotten that bit and then updated msdn later.

Comment 10

[Jim Mathies [:jimm]]

Note this bug is marked leave-open so we can see if the patch I've landed helps. There may be other issues to sort out.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f5234c69f58

Comment 12

[Jim Mathies [:jimm]]

Hey Brian, curious about something - if there's an update that's staged and I hit the restart button, which process does the actual copying of the new install over? Is there an interim process that launches, or does the update service handle this somehow?

(Also, for testing over the break, can I use oak to generate some nightlies?)

Comment 13

[Brian R. Bondy [:bbondy]]

So when you hit update from Desktop:
- Process shuts down, just before existing the program launches a new firefox.exe process
- On firefox.exe startup it checks if there is an update stated. If so it processes it by launching updater.exe to do the update.
- After updater.exe is run it relaunches Firefox.

When you hit update from Metro:
- Process shuts down and notifies to CEH to restart firefox.exe
- CEH waits until process is closed then relaunches firefox.exe in metro mode
- startup code detects update happens and starts updater.exe to do the update.
- updater.exe relaunches Metro Firefox.

Comment 14

[Brian R. Bondy [:bbondy]]

Feel free to use oak by the way:
You'll probably want to rebase to the tip of m-c:

cd oak-checkour-dir
hg pull m-c-url
hg heads .
hg update -r revisin_to_forget_about (i.e. old oak tip)
hg commit --close-branch
hg update -r revision_to_use (i.e m-c tip)
hg push --force

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 8348662 [details] [diff] [review]
remove no splash option

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 893784
User impact if declined: buggy restarts
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/361c5c041452

Comment 17

[Jim Mathies [:jimm]]

> When you hit update from Metro:
> 1 Process shuts down and notifies to CEH to restart firefox.exe
> 2 CEH waits until process is closed then relaunches firefox.exe in metro mode
> 3 startup code detects update happens and starts updater.exe to do the update.
> 4 updater.exe relaunches Metro Firefox.


The patch that landed fixes the restart to update problem 1-3). But I'm still seeing a failed restart (4) after the update completes.

I wonder if there's some way we can run updater directly at #2, rather than relaunch the whole browser. Then relaunch firefox after that completes.

Comment 18

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #17)
> > When you hit update from Metro:
> > 1 Process shuts down and notifies to CEH to restart firefox.exe
> > 2 CEH waits until process is closed then relaunches firefox.exe in metro mode
> > 3 startup code detects update happens and starts updater.exe to do the update.
> > 4 updater.exe relaunches Metro Firefox.
> 
> 
> The patch that landed fixes the restart to update problem (1-3). But I'm
> still seeing a failed restart (4) after the update completes.
> 
> I wonder if there's some way we can run updater directly at #2, rather than
> relaunch the whole browser. Then relaunch firefox after that completes.

I'm going to concentrate on getting step 4 working first. We can file a follow up on the middle splash screen, which isn't a deal killer, it's just annoying.

Comment 19

[Brian R. Bondy [:bbondy]]

Yep it works the same as the desktop, I would try to keep things the same for updates from Metro and Desktop.  Since the splash screen thing is required now but wasn't before, it may make sense to launch the Desktop browser though with a command line that will be passed to updater and can be checked to launch Metro instead on relaunch.
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#1732

Comment 20

[Jim Mathies [:jimm]]

Interesting thing I'm seeing is that we get the second restart and the browser launches, but for some reason, it's forced into the background in a suspended state. For example:

1) check about for updates
2) download update
3) hit restart button

browser closes
browser relaunches (splash screen)
browser closes again
no relaunch (no splash screen)

checking the app bar though shows a nightly app, and selecting that opens the browser.

tracing this using DebugView.

Comment 21

[Jim Mathies [:jimm]]

Attachment: part 1 - metroutils update restart flag

Comment 22

[Jim Mathies [:jimm]]

Attachment: part 2 - MetroAppShell restart params

Comment 23

[Jim Mathies [:jimm]]

Attachment: part 3 - ceh helper changes

Comment 24

[Jim Mathies [:jimm]]

Attachment: part 4 - get rid of delayed restart

Now that we restart after xpcom shutdown I don't think we need this anymore.

Comment 25

[Jim Mathies [:jimm]]

Attachment: part 5 Replace misc. boolean flags and cleanup CEH Execute

Comment 26

[Jim Mathies [:jimm]]

Attachment: part 6 - update updater restart checks

Comment 27

[Jim Mathies [:jimm]]

Attachment: part 7 - front end update flag

Comment 29

[Brian R. Bondy [:bbondy]]

Comment on attachment 8363003 [details] [diff] [review]
part 7 - front end update flag

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

::: browser/metro/base/content/flyoutpanels/AboutFlyoutPanel.js
@@ +277,5 @@
>          appStartup.restartInSafeMode(Components.interfaces.nsIAppStartup.eAttemptQuit);
>          return;
>        }
>  
> +      Services.metro.updatePending = true;

Why can't we just query appStartup for this?
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/startup/public/nsIAppStartup.idl?from=nsIAppStartup.idl#180

Comment 30

[Brian R. Bondy [:bbondy]]

Comment on attachment 8362974 [details] [diff] [review]
part 1 - metroutils update restart flag

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

::: widget/nsIWinMetroUtils.idl
@@ +37,5 @@
> +   * Helper for our restart logic up in the about flyout. We set this
> +   * right before we restart for an update so that MetroAppShell can
> +   * communicate this to the ceh.
> +   */
> +  attribute boolean updatePending;

Why can't we just query appStartup for this?
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/startup/public/nsIAppStartup.idl?from=nsIAppStartup.idl#180

Comment 31

[Brian R. Bondy [:bbondy]]

Comment on attachment 8362981 [details] [diff] [review]
part 5 Replace misc. boolean flags and cleanup CEH Execute

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

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +740,5 @@
> +  // Deal with metro restart for an update - launch desktop with a command
> +  // that tells it to run updater then launch the metro browser.
> +  if (mRequestType == METRO_UPDATE) {
> +    mParameters = kMetroUpdateCmdLine;
> +    LaunchDesktopBrowser();

Wouldn't it be possible for the Desktop browser to launch updater, and the update to finish, all before the metro browser is properly closed. Then after the update, the code tries to launch metro, which is already open, so it is brought to the foreground.  But then it is closed on the user?

Comment 32

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #30)
> Comment on attachment 8362974 [details] [diff] [review]
> part 1 - metroutils update restart flag
> 
> Review of attachment 8362974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/nsIWinMetroUtils.idl
> @@ +37,5 @@
> > +   * Helper for our restart logic up in the about flyout. We set this
> > +   * right before we restart for an update so that MetroAppShell can
> > +   * communicate this to the ceh.
> > +   */
> > +  attribute boolean updatePending;
> 
> Why can't we just query appStartup for this?
> http://dxr.mozilla.org/mozilla-central/source/toolkit/components/startup/
> public/nsIAppStartup.idl?from=nsIAppStartup.idl#180

Thought about that, but then there's the issue of making it valid on all platforms, which seemed like a real pita. So I put it in utils instead.

Comment 33

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #31)
> Comment on attachment 8362981 [details] [diff] [review]
> part 5 Replace misc. boolean flags and cleanup CEH Execute
> 
> Review of attachment 8362981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
> @@ +740,5 @@
> > +  // Deal with metro restart for an update - launch desktop with a command
> > +  // that tells it to run updater then launch the metro browser.
> > +  if (mRequestType == METRO_UPDATE) {
> > +    mParameters = kMetroUpdateCmdLine;
> > +    LaunchDesktopBrowser();
> 
> Wouldn't it be possible for the Desktop browser to launch updater, and the
> update to finish, all before the metro browser is properly closed. Then
> after the update, the code tries to launch metro, which is already open, so
> it is brought to the foreground.  But then it is closed on the user?

I suppose that could happen. I could reintroduce that running process code check in part 4 to be 100% safe here. Execute is called sync from the old instance so maybe that's a good idea.

Comment 34

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #33)
> (In reply to Brian R. Bondy [:bbondy] from comment #31)
> > Comment on attachment 8362981 [details] [diff] [review]
> > part 5 Replace misc. boolean flags and cleanup CEH Execute
> > 
> > Review of attachment 8362981 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
> > @@ +740,5 @@
> > > +  // Deal with metro restart for an update - launch desktop with a command
> > > +  // that tells it to run updater then launch the metro browser.
> > > +  if (mRequestType == METRO_UPDATE) {
> > > +    mParameters = kMetroUpdateCmdLine;
> > > +    LaunchDesktopBrowser();
> > 
> > Wouldn't it be possible for the Desktop browser to launch updater, and the
> > update to finish, all before the metro browser is properly closed. Then
> > after the update, the code tries to launch metro, which is already open, so
> > it is brought to the foreground.  But then it is closed on the user?
> 
> I suppose that could happen. I could reintroduce that running process code
> check in part 4 to be 100% safe here. Execute is called sync from the old
> instance so maybe that's a good idea.

Ya that has happened in the past and that's part of why that code existed specifically

Comment 35

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #32)
> (In reply to Brian R. Bondy [:bbondy] from comment #30)
> > Comment on attachment 8362974 [details] [diff] [review]
> > part 1 - metroutils update restart flag
> > 
> > Review of attachment 8362974 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/nsIWinMetroUtils.idl
> > @@ +37,5 @@
> > > +   * Helper for our restart logic up in the about flyout. We set this
> > > +   * right before we restart for an update so that MetroAppShell can
> > > +   * communicate this to the ceh.
> > > +   */
> > > +  attribute boolean updatePending;
> > 
> > Why can't we just query appStartup for this?
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/components/startup/
> > public/nsIAppStartup.idl?from=nsIAppStartup.idl#180
> 
> Thought about that, but then there's the issue of making it valid on all
> platforms, which seemed like a real pita. So I put it in utils instead.

Do you mean valid on all platforms when the metro front end is in use? If so we don't need to support that.

Comment 36

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #35)
> (In reply to Jim Mathies [:jimm] from comment #32)
> > (In reply to Brian R. Bondy [:bbondy] from comment #30)
> > > Comment on attachment 8362974 [details] [diff] [review]
> > > part 1 - metroutils update restart flag
> > > 
> > > Review of attachment 8362974 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: widget/nsIWinMetroUtils.idl
> > > @@ +37,5 @@
> > > > +   * Helper for our restart logic up in the about flyout. We set this
> > > > +   * right before we restart for an update so that MetroAppShell can
> > > > +   * communicate this to the ceh.
> > > > +   */
> > > > +  attribute boolean updatePending;
> > > 
> > > Why can't we just query appStartup for this?
> > > http://dxr.mozilla.org/mozilla-central/source/toolkit/components/startup/
> > > public/nsIAppStartup.idl?from=nsIAppStartup.idl#180
> > 
> > Thought about that, but then there's the issue of making it valid on all
> > platforms, which seemed like a real pita. So I put it in utils instead.
> 
> Do you mean valid on all platforms when the metro front end is in use? If so
> we don't need to support that.

I want to avoid adding a new Windows metro only flag that might confuse people. For example, a flag like 'eUpdatePending' seems sloppy if it's only supported on Windows. 

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/public/nsIAppStartup.idl#97

Comment 37

[Brian R. Bondy [:bbondy]]

We have some code in toolkit for determining if update is pending or not, for example:
http://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp?from=GetUpdateStatus#238

There's probably some other places too, not sure if that's better or not for this case though, a simple flag seems simpler. Pls check with rstrong to see what he thinks about it.

Comment 38

[Jim Mathies [:jimm]]

Attachment: part 1 - metroutils update restart flag

Comment 39

[Jim Mathies [:jimm]]

Attachment: part 2 - MetroAppShell restart params

Comment 40

[Jim Mathies [:jimm]]

Attachment: part 3 - ceh helper changes

Comment 41

[Jim Mathies [:jimm]]

Attachment: part 4 - remove threaded delay restart

Comment 42

[Jim Mathies [:jimm]]

Attachment: part 5 Replace misc. boolean flags and cleanup CEH Execute

Comment 43

[Jim Mathies [:jimm]]

Attachment: part 6 - update nsUpdateDriver restart check

Comment 44

[Jim Mathies [:jimm]]

Attachment: part 7 - set the front end update pending flag in the about flyout

Comment 45

[Jim Mathies [:jimm]]

Attachment: part8 - delay desktop update restart until the metro browser shuts down

Comment 46

[Jim Mathies [:jimm]]

Attachment: part 9 - nsAppRunner support for metroupdates

Comment 47

[Jim Mathies [:jimm]]

Comment on attachment 8366663 [details] [diff] [review]
part 7 - set the front end update pending flag in the about flyout

Per comment 30, comment 32, and comment 36. Seeking feedback on this change. The alternative here would be to add a new flag to nsIAppStartup that might be metro specific which marks the restart as having a pending update. I went with a flag in our metro utils which our app shell picks up on.

Comment 48

[Jim Mathies [:jimm]]

Two things I'm wondering about here - 

1) The amount of time we spend at the Windows start screen isn't long but it's long enough that some users might try to relaunch the browser before the update completes and the metro browser launches. Seems like that could really screw up the update. I wonder if there's a need for the CEH to query the update service to see if an update is in progress, and if so, delay the request to launch until complete. Note this wouldn't avoid post update processing, which might be a concern as well.

2) What happens if the update service is disabled with metro updates? Do we update or do we defer to desktop?

Comment 49

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #48)
> Two things I'm wondering about here - 
> 
> 1) The amount of time we spend at the Windows start screen isn't long but
> it's long enough that some users might try to relaunch the browser before
> the update completes and the metro browser launches. Seems like that could
> really screw up the update. I wonder if there's a need for the CEH to query
> the update service to see if an update is in progress, and if so, delay the
> request to launch until complete. Note this wouldn't avoid post update
> processing, which might be a concern as well.

We lock the firefox.exe file during update so this shouldn't be possible.  I'm not sure exactly what happens in Metro mode, maybe it just doesn't do anything when you click the tile?

> 
> 2) What happens if the update service is disabled with metro updates? Do we
> update or do we defer to desktop?

We still update as normal but it just doens't use the service, it still does the update initiated through the metro front end.  I tested this previously and it gives you the UAC prompt no problem.  I haven't re-tested lately though.

Comment 50

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #49)
> (In reply to Jim Mathies [:jimm] from comment #48)
> > Two things I'm wondering about here - 
> > 
> > 1) The amount of time we spend at the Windows start screen isn't long but
> > it's long enough that some users might try to relaunch the browser before
> > the update completes and the metro browser launches. Seems like that could
> > really screw up the update. I wonder if there's a need for the CEH to query
> > the update service to see if an update is in progress, and if so, delay the
> > request to launch until complete. Note this wouldn't avoid post update
> > processing, which might be a concern as well.
> 
> We lock the firefox.exe file during update so this shouldn't be possible. 
> I'm not sure exactly what happens in Metro mode, maybe it just doesn't do
> anything when you click the tile?

Does the type of lock we use prevent the exe from executing? If so I guess our activation call to Windows would fail in the CEH and the ceh would shutdown, or try to launch the desktop browser instead, which might also fail.

> > 2) What happens if the update service is disabled with metro updates? Do we
> > update or do we defer to desktop?
> 
> We still update as normal but it just doens't use the service, it still does
> the update initiated through the metro front end.  I tested this previously
> and it gives you the UAC prompt no problem.  I haven't re-tested lately
> though.

Ok, thanks. I'll test this with my oak builds.

Comment 51

[Brian R. Bondy [:bbondy]]

> Does the type of lock we use prevent the exe from executing?

Yes

> If so I guess our activation call to Windows would fail in the CEH and the ceh would shutdown,
> or try to launch the desktop browser instead, which might also fail.

Yep, that's what I would expect. 

To test this you can just create a small file that does a CreateFile with no reading sharing flags specified on firefox.exe.  With the handle open, popup a MessageBox.  Then try to launch it via the tile. 

Here's the actual code that does this same thing 
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#3014

Comment 52

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #51)
> > Does the type of lock we use prevent the exe from executing?
> 
> Yes
> 
> > If so I guess our activation call to Windows would fail in the CEH and the ceh would shutdown,
> > or try to launch the desktop browser instead, which might also fail.
> 
> Yep, that's what I would expect. 
> 
> To test this you can just create a small file that does a CreateFile with no
> reading sharing flags specified on firefox.exe.  With the handle open, popup
> a MessageBox.  Then try to launch it via the tile. 
> 
> Here's the actual code that does this same thing 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/
> updater.cpp#3014

Ok, looking at the code, we'll try to active then exit. So maybe we could test for the lock and avoid activating. Then when the update completes we'll launch.

Comment 53

[Jim Mathies [:jimm]]

Attachment: part 10 - delay launch during updates

Note, if the user does do this, both the updater and the ceh will launch. From testing this isn't an issue, I fired off a bunch of ceh launchers while holding a ten second lock on firefox, and everything worked fine.

This patch also fixes an old ref count bug in the ceh, it was sticking around too long after launch.

Comment 54

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

Comment on attachment 8366663 [details] [diff] [review]
part 7 - set the front end update pending flag in the about flyout

I think this makes sense on Metro though Brian has a better understanding of this UI and should look at the changes.

Comment 55

[Brian R. Bondy [:bbondy]]

You can r=me with the commenting out

Comment 56

[Jim Mathies [:jimm]]

Comment on attachment 8366654 [details] [diff] [review]
part 1 - metroutils update restart flag

Woot, now the fun begins!

Adding the updatePending flag to metro utils.

Comment 57

[Jim Mathies [:jimm]]

Comment on attachment 8366655 [details] [diff] [review]
part 2 - MetroAppShell restart params

Add a new command line parameter for the ceh call to restart when metro is updating. Plus a little cleanup.

Comment 58

[Jim Mathies [:jimm]]

Comment on attachment 8366659 [details] [diff] [review]
part 3 - ceh helper changes

This - 

1) adds a registry dump flag so we can get at ceh output even when there's no debugger attached via tools like debugview.
2) adds a couple running process helpers.

Comment 59

[Jim Mathies [:jimm]]

Comment on attachment 8366660 [details] [diff] [review]
part 4 - remove threaded delay restart

Remove the threaded restart logic. Part 8 brings this back, but it uses a timer callback on CExecuteCommandVerb so the callback has access to everything in CExecuteCommandVerb.

Comment 60

[Jim Mathies [:jimm]]

Comment on attachment 8366661 [details] [diff] [review]
part 5 Replace misc. boolean flags and cleanup CEH Execute

This:

1) adds support for --metro-update in the ceh
2) replaces all of our restart related boolean flags with an enum.

Comment 61

[Jim Mathies [:jimm]]

Comment on attachment 8366662 [details] [diff] [review]
part 6 - update nsUpdateDriver restart check

When we restart the desktop browser to do a metro update, we pass in the new command line flag '--metro-update'. This adds support for detecting this flag in nsUpdateDriver.

Comment 62

[Jim Mathies [:jimm]]

Comment on attachment 8366664 [details] [diff] [review]
part8 - delay desktop update restart until the metro browser shuts down

This brings back delayed start for the desktop browser when doing a metro update.

Comment 63

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

Comment on attachment 8366662 [details] [diff] [review]
part 6 - update nsUpdateDriver restart check

Brian, can you take this?

Comment 64

[Jim Mathies [:jimm]]

Comment on attachment 8366665 [details] [diff] [review]
part 9 - nsAppRunner support for metroupdates

nsAppRunner changes to pick up the --metro-update flag and process updates, then exit. Also updates when we set the last run type.

Comment 65

[Jim Mathies [:jimm]]

Comment on attachment 8366745 [details] [diff] [review]
part 10 - delay launch during updates

Last but not least, don't let the browser launch if an update it in the works, plus a ref count fix previously mentioned.

Comment 66

[Brian R. Bondy [:bbondy]]

Comment on attachment 8366662 [details] [diff] [review]
part 6 - update nsUpdateDriver restart check

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

::: toolkit/xre/nsUpdateDriver.cpp
@@ +830,5 @@
>    }
>  
>    int immersiveArgc = 0;
>  #ifdef XP_WIN
> +  if (IsWindowsMetroUpdateRequest(appArgc, appArgv)) {

nit: It would probably be nice to also have a ifdef MOZ_METRO here for other apps that use this code and for the rest of the patch.

Comment 67

[Brian R. Bondy [:bbondy]]

Comment on attachment 8366665 [details] [diff] [review]
part 9 - nsAppRunner support for metroupdates

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

::: toolkit/xre/nsAppRunner.cpp
@@ +2804,5 @@
>  #ifdef USE_GLX_TEST
>  bool fire_glxtest_process();
>  #endif
>  
> +#ifdef XP_WIN

nit: It would be nice to also have MOZ_METRO flags here and elsewhere in the patch.

Comment 68

[Brian R. Bondy [:bbondy]]

Comment on attachment 8366745 [details] [diff] [review]
part 10 - delay launch during updates

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

r=me with the change

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +703,5 @@
> +    return false;
> +  }
> +
> +  HANDLE hFile = CreateFileW(browserPath,
> +                             FILE_EXECUTE, 0,

I think we want to specify sharing flags from here. We don't want to lock the binary during any period from here ourselves if it's not already in use.

Comment 69

[Jim Mathies [:jimm]]

Updated per comments and pushed to oak.

Comment 70

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ab0c3b5fab
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e4bcdfa549
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cca152c157c7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/294253bf4a5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0742872e6a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/748cfe2f9383
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b2704a550c02
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1295f111dbd9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ccb813dcd1d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a41fdfce8810

Comment 71

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3ab0c3b5fab
https://hg.mozilla.org/mozilla-central/rev/d1e4bcdfa549
https://hg.mozilla.org/mozilla-central/rev/cca152c157c7
https://hg.mozilla.org/mozilla-central/rev/294253bf4a5f
https://hg.mozilla.org/mozilla-central/rev/5b0742872e6a
https://hg.mozilla.org/mozilla-central/rev/748cfe2f9383
https://hg.mozilla.org/mozilla-central/rev/b2704a550c02
https://hg.mozilla.org/mozilla-central/rev/1295f111dbd9
https://hg.mozilla.org/mozilla-central/rev/9ccb813dcd1d
https://hg.mozilla.org/mozilla-central/rev/a41fdfce8810

Comment 72

[u279076]

Kamil, can you please test to confirm this is resolved?

Comment 73

[Gary [:streetwolf52]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c0ee7e389205  - Good
http://hg.mozilla.org/integration/mozilla-inbound/rev/a41fdfce8810  - Bad

Even though I get this error and the one that Nightly needs to be closed replying yes keeps me in Nightly with no Nightly crash.

Event Log:

Faulting application name: CommandExecuteHandler.exe, version: 29.0.0.5142, time stamp: 0x52eab06b
Faulting module name: SHELL32.dll, version: 6.3.9600.16474, time stamp: 0x52902a5b
Exception code: 0xc0000005
Fault offset: 0x001c6322
Faulting process id: 0x10d8
Faulting application start time: 0x01cf1ed7b44a29e6
Faulting application path: C:\Users\Gary\My Programs\firefox\CommandExecuteHandler.exe
Faulting module path: C:\Windows\SYSTEM32\SHELL32.dll
Report Id: f28bde87-8aca-11e3-83f3-c86000a0a026
Faulting package full name: 
Faulting package-relative application ID:

Comment 74

[Gary [:streetwolf52]]

This is using Fx29 Desktop version on Windows 8.1 Pro x64.

Comment 75

[Gary [:streetwolf52]]

Wish you could edit replies.  I only get this error if I click on a link from an external source such as an email in Outlook.

Comment 76

[u279076]

(In reply to Gary [:streetwolf] from comment #74)
> This is using Fx29 Desktop version on Windows 8.1 Pro x64.

Thanks, can you confirm this by using the Nightly from today, not the integration branch?

Comment 77

[Gary [:streetwolf52]]

Nightly http://hg.mozilla.org/mozilla-central/rev/735a648bca0d does the same thing except it produces one less error message.

Comment 78

[Gary [:streetwolf52]]

I believe this was a respin of Nightly.

Comment 79

[Gary [:streetwolf52]]

I was wrong.  I still get this popup in addition to the Nightly must close message.

---------------------------
CommandExecuteHandler.exe - Application Error
---------------------------
The instruction at 0x75616322 referenced memory at 0x6a9c2494. The memory could not be read.


Click on OK to terminate the program
---------------------------
OK   
---------------------------

Comment 80

[u279076]

Thanks Gary.

Kamil, can you please test this to confirm what Gary is seeing?

Comment 81

[Gary [:streetwolf52]]

Let me add that I tested with a new profile.

Comment 82

[Jim Mathies [:jimm]]

(In reply to Gary [:streetwolf] from comment #73)
> Faulting application name: CommandExecuteHandler.exe, version: 29.0.0.5142,
> time stamp: 0x52eab06b
> Faulting module name: SHELL32.dll, version: 6.3.9600.16474, time stamp:
> 0x52902a5b
> Exception code: 0xc0000005
> Fault offset: 0x001c6322
> Faulting process id: 0x10d8
> Faulting application start time: 0x01cf1ed7b44a29e6
> Faulting application path: C:\Users\Gary\My
> Programs\firefox\CommandExecuteHandler.exe
> Faulting module path: C:\Windows\SYSTEM32\SHELL32.dll
> Report Id: f28bde87-8aca-11e3-83f3-c86000a0a026
> Faulting package full name: 
> Faulting package-relative application ID:

Hey Gary, could you file a new bug on this please and cc me in (:jimm)?

Comment 83

[Jim Mathies [:jimm]]

Due to bug 966626, I've cancelled the 02-01 Windows nightly.

Comment 84

[Jim Mathies [:jimm]]

I retriggered nightlies for this.

Comment 85

[Kamil Jozwiak [:kjozwiak]]

Went through the following verification process without any issues. Used the following builds:
- Installed the following: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-02-03-02-04-mozilla-central/
- Once installed, updated the build to the latest Nightly using both Desktop/Metro updaters under "About"

- Ensured that restarting the browser after an update under the metro environment relaunches Firefox Metro without any issues
- Ensured that restarting the browser after an update under the desktop environment relaunches Firefox Desktop without any issues
- Ensured that selecting the "Nightly" tile after an update launches the browser without any issues
- Dismissed the "About" flyout while the download was in progress, returned several minutes later and ensured that the download/update has been completed without issues
- Ensured that closing Firefox Metro several times while downloading the update didn't cause any issues once the update was applied
- When Firefox Metro is set as the default environment, ensured that external links launch Firefox Metro without any issues (while browser is closed/opened)
- When Firefox Desktop is set as the default environment, ensured that external links launch Firefox Desktop without any issues (while browser is closed/opened)
- Went through the case outlined in comment #48 and ensured that taping on the Nightly tile while the update is in progress didn't cause major issues

Issues found while going through verification:

- When you run through the case outlined in comment #48, the Firefox Metro splash screen will quickly appear/disappear. At this point Firefox Metro will not relaunch again but will appear as an opened application under the "Opened App List". Taping on the Firefox Metro splash screen thumbnail under the "Opened App List" will relaunch the updated Firefox Metro. This looks like a corner case that is similar to the original issue outlined in comment #0 (Created Bug #)

- When you're downloading an update via the Firefox Metro "About" flyout, dismissing the flyout and then returning to it (give it enough time to complete the entire download) will seem like the download has completely stalled. The number of MB's downloaded will be the exactly the same when the flyout was closed but won't increment giving it the appearance that the download process has forzen/stalled. In reality, the update was already downloaded and applied and a restart is required (but there's no indication under the "About" flyout. If you close the browser and re-open it after about one minute, the new updated Firefox Metro will launch. (Created Bug # 967394)

I will go through some more test cases tomorrow and finish this off as its getting really late here!

Comment 86

[Kamil Jozwiak [:kjozwiak]]

Went through the following verification process without any issues. Used the following builds:
- Installed the following: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-02-03-02-04-mozilla-central/
- Once installed, updated the build to the latest Nightly using both Desktop/Metro updaters under "About"

- Went through several updates and switched between the two environments and couldn't reproduce the issue outlined in comment # 74 (never received a prompt/crash)
- Updated Firefox Metro via different variations of snapped view and ensured that Firefox Metro is launched the first time around the tile is taped/clicked
- Updating Firefox Metro while having several tabs opened, ensured that the browser opened the first time the Firefox Metro tile is taped/clicked
- Tested opening links via third party applications more extensively (Facebook, Twitter, Mail, App Store, Evernote Touch)

Marking as verified as Firefox Metro always runs the first time around once an update is applied. There are several issues with the Firefox Metro splash screen being left behind, but these issues are related to switching between the desktop/metro environment.

Comment 87

[Jim Mathies [:jimm]]

Comment on attachment 8366745 [details] [diff] [review]
part 10 - delay launch during updates

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long standing update bug in metrofx.
User impact if declined: Buggy restarts when updating.
Testing completed (on m-c, etc.): yes, verified fixed.
Risk to taking this patch (and alternatives if risky): Medium, this involves changes to how we update from within the metrofx browser. However we've had this on mc now for a week without issue (except bug 966626, which I'll also flag).
String or IDL/UUID changes made by this patch: none

This is a beta uplift request for parts 1-10 here, plus the patch in follow up fix bug 966626.

Comment 88

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jim Mathies [:jimm] from comment #87)
> String or IDL/UUID changes made by this patch: none

Patch 1 makes IDL/UUID changes. Does this have ba+?

Comment 89

[Jim Mathies [:jimm]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #88)
> (In reply to Jim Mathies [:jimm] from comment #87)
> > String or IDL/UUID changes made by this patch: none
> 
> Patch 1 makes IDL/UUID changes. Does this have ba+?

Ah yes, sorry. This is an internal interface and we don't support addons so I don't think there's an issue here.

Comment 90

[Jim Mathies [:jimm]]

Note, bug 966626 must get uplifted with this patch set.

Comment 91

[Ryan VanderMeulen [:RyanVM]]

ba=jimm WFM!

https://hg.mozilla.org/releases/mozilla-beta/rev/6673ba8dfffd
https://hg.mozilla.org/releases/mozilla-beta/rev/5e595adf2d9e
https://hg.mozilla.org/releases/mozilla-beta/rev/07c25785ec67
https://hg.mozilla.org/releases/mozilla-beta/rev/dddf11dc7526
https://hg.mozilla.org/releases/mozilla-beta/rev/5b3201c97b29
https://hg.mozilla.org/releases/mozilla-beta/rev/10bed2ab6575
https://hg.mozilla.org/releases/mozilla-beta/rev/25dd5a9531d3
https://hg.mozilla.org/releases/mozilla-beta/rev/5d2ffbeb04f7
https://hg.mozilla.org/releases/mozilla-beta/rev/138ec444b4e3
https://hg.mozilla.org/releases/mozilla-beta/rev/870cd04b8bfd

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/6673ba8dfffd
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5e595adf2d9e
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/07c25785ec67
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/dddf11dc7526
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5b3201c97b29
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/10bed2ab6575
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/25dd5a9531d3
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5d2ffbeb04f7
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/138ec444b4e3
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/870cd04b8bfd

Comment 93

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0

Verified using the STR from comment 85 and comment 86 with a Surface Pro 2 device on Firefox 28 Beta 4 (build ID: 20140218122424).