Closed Bug 874323 Opened 7 years ago Closed 7 years ago

Defect - Metro does not restart itself ater closing after an update because Ci.nsIAppStartup.eRestart is not supported in Metro

Categories

(Toolkit :: Application Update, defect, P1)

x86_64
Windows 8.1
defect

Tracking

()

VERIFIED FIXED
mozilla24

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3)

Attachments

(3 files, 1 obsolete file)

After an update from the Metro interface occurs, we have a restart to update prompt.  This tries to use Components.interfaces.nsIAppStartup.eRestart which has no effect from Metro.

I think we need to return NS_SUCCESS_RESTART_APP from xre metro main or something like that.
Summary: We aren't handling Components.interfaces.nsIAppStartup.eRestart from Metro → Metro does not restart itself ater closing after an update because Ci.nsIAppStartup.eRestart is not supported in Metro
Priority: -- → P1
QA Contact: jbecerra
Summary: Metro does not restart itself ater closing after an update because Ci.nsIAppStartup.eRestart is not supported in Metro → Defect - Metro does not restart itself ater closing after an update because Ci.nsIAppStartup.eRestart is not supported in Metro
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3
I tried a couple different things here yesterday but they didn't work:

1. Using the activation manager to restart Metro Firefox at the right time under terminations of this type.  This did not work because it just brings metro to the front instead of opening a new instance.

2. Instead starting Desktop Firefox via WinLaunch Child, this doesn't work because Metro is not started after the update is completed. (Wrong profile in use).

I think to move forward we need to just launch updater.exe directly. Robert do you see any problems with just launching updater.exe directly on shutdown instead of on startup (only for Metro)?
Flags: needinfo?(robert.bugzilla)
Besides the obvious problem of dealing with OS shutdown I don't see any problems though changing this does make me very nervous.
Flags: needinfo?(robert.bugzilla)
Ya I was thinking we did the restart to launch updater to ensure a cleaner/easier/non-hung shutdown before performing the update. 

But this change will be from the Metro browser only, so in the worst case if something goes wrong people can still update as normal through the Desktop interface.
Status: NEW → ASSIGNED
Carrying over to Iteration #8.
Status: ASSIGNED → NEW
Blocks: metrov1it8
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
I'll attach another patch for review for increasing the timeout for the metro case (and not aborting the update when the process is in use) as discussed at today's 1:1.  Tested this on Oak and it's working.
Attachment #756758 - Flags: review?(robert.bugzilla)
Attachment #756771 - Flags: review?(robert.bugzilla)
Attachment #756758 - Attachment description: Patch v1. → Patch v1 - Allow UpdateProcessor to do application switches
Comment on attachment 756771 [details] [diff] [review]
Patch v1 - Make updater wait longer for Metro

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -1672,16 +1676,33 @@ PatchIfFile::Finish(int status)
> 
> //-----------------------------------------------------------------------------
> 
> #ifdef XP_WIN
> #include "nsWindowsRestart.cpp"
> #include "nsWindowsHelpers.h"
> #include "uachelper.h"
> #include "pathhash.h"
>+
>+#ifdef MOZ_METRO
>+/**
>+ * Determines if the update came from an Immersive browser
>+ * @return true if the update came from an immersive browser
>+ */
>+bool
>+IsUpdateFromMetro(int argc, NS_tchar **argv)
>+{
>+  for (int i = 0; i < argc; i++) {
>+    if (!wcsicmp(L"-ServerName:DefaultBrowserServer", argv[i])) {
>+      return true;
>+    }
>+  }
>+  return false;
>+}
>+#endif
> #endif
> 
> static void
> LaunchCallbackApp(const NS_tchar *workingDir, 
>                   int argc, 
>                   NS_tchar **argv, 
>                   bool usingService)
> {

>@@ -2461,19 +2480,22 @@ int NS_main(int argc, NS_tchar **argv)
> 
> #ifdef XP_WIN
>   if (pid > 0) {
>     HANDLE parent = OpenProcess(SYNCHRONIZE, false, (DWORD) pid);
>     // May return NULL if the parent process has already gone away.
>     // Otherwise, wait for the parent process to exit before starting the
>     // update.
>     if (parent) {
>-      DWORD result = WaitForSingleObject(parent, 5000);
>+      bool updateFromMetro = IsUpdateFromMetro(argc, argv);
IsUpdateFromMetro is ifdef'd MOZ_METRO and this is only ifdef'd XP_WIN

>+      DWORD waitTime = updateFromMetro ?
>+                       IMMERSIVE_PARENT_WAIT : PARENT_WAIT;
>+      DWORD result = WaitForSingleObject(parent, waitTime);
>       CloseHandle(parent);
>-      if (result != WAIT_OBJECT_0)
>+      if (result != WAIT_OBJECT_0 && !updateFromMetro)
>         return 1;
>     }
>   }

I want one more pass on this
Attachment #756771 - Flags: review?(robert.bugzilla) → review-
Review comment implemented
Attachment #756771 - Attachment is obsolete: true
Attachment #760544 - Flags: review?(robert.bugzilla)
This happens from the unelevated updater process which just sits around to relaunch firefox after the update, so there is no helpful place to log any failures here. 

The main problem in this bug is to deal with actually restarting firefox from firefox, in order to initiate an update.  But this patch is about restarting firefox after updater.exe runs.  I noticed that it also sometimes doesn't restart, and I think this may help in some situations.
Attachment #760551 - Flags: review?(jmathies)
Attachment #760551 - Flags: review?(jmathies) → review+
Blocks: metrov1it9
No longer blocks: metrov1it8
Target Milestone: --- → mozilla24
Blocks: metrov1it8
No longer blocks: metrov1it9
Can any one provide me steps to reproduce? I want to check this for iteration-9. Thanks!
Hey Brian, is it possible for QA to verify this defect (see Comment 11)?
Flags: needinfo?(netzen)
Just making sure after you go to the about flyout and click to restart it actually restarts and applies the update.  Just testing the story should be sufficient.
Flags: needinfo?(netzen)
I successfully upgraded Nightly from Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130708 Firefox/25.0 to Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130709 Firefox/25.0 in Metro mode: using the guidance from comment #13: Nightly restarted and update applied when using the restart to update button in the about flyout.
Marking verified.
Status: RESOLVED → VERIFIED
(In reply to Brian R. Bondy [:bbondy] from comment #13)
> Just making sure after you go to the about flyout and click to restart it
> actually restarts and applies the update.  Just testing the story should be
> sufficient.

Hi Brian, yes It was working when I applied update from about flyout and restarting properly with updated nightly. Mihaela has verified it so, I am not providing full details.
I've had a couple problems with this over the weekend, is there another open bug on this?

Three different outcomes when pressing restart after an update applies -

1) the browser shuts down and restarts.
2) the restart button receives focus but nothing happens.
3) the browser shuts down on pressing the restart button and doesn't restart.

On #2 and #3, closing the browser manually and launching: the browser shuts down immediately on the manual launch, then launches on its own a second later.

This is on a clean machine, no dev tools. I've been updating it regularly. I'd rather not install any tools, but I can copy logs off it or turn on nspr logging to collect info if it can help diagnose.
Also, I've noticed that content is active under the about flyout.
(In reply to Jim Mathies [:jimm] from comment #17)
> Also, I've noticed that content is active under the about flyout.

Sounds like a general flyout problem and not related to this bug and updates right?
(Just making sure I understand correctly)

> Three different outcomes when pressing restart after an update applies -
>
> 1) the browser shuts down and restarts.
> 2) the restart button receives focus but nothing happens.
> 3) the browser shuts down on pressing the restart button and doesn't restart.

So either #1 happens or #2 or #3 but not all (because each doesn't make sense combined with the other I think). Did you notice any difference about when each happens? Or you just see one of the above 3 happen randomly?
(In reply to Brian R. Bondy [:bbondy] from comment #18)
> (In reply to Jim Mathies [:jimm] from comment #17)
> > Also, I've noticed that content is active under the about flyout.
> 
> Sounds like a general flyout problem and not related to this bug and updates
> right?
> (Just making sure I understand correctly)
> 
> > Three different outcomes when pressing restart after an update applies -
> >
> > 1) the browser shuts down and restarts.
> > 2) the restart button receives focus but nothing happens.
> > 3) the browser shuts down on pressing the restart button and doesn't restart.
> 
> So either #1 happens or #2 or #3 but not all (because each doesn't make
> sense combined with the other I think). Did you notice any difference about
> when each happens? Or you just see one of the above 3 happen randomly?

right. I've had all three happen at one time or another. #3 doesn't appear to be related to the flyout, I think I got that on Friday. #2 happened to me twice over the weekend.
I think I've seen #3 but not #2. I think #2 and #3 need to be posted as different defects, #1 I think is it working correctly.  For the other flyout general problem accepting input behind it I think we need a new bug for this but we've had similar ones posted about the appbar before.

I'll post for #2 and #3, I'll leave the 'accepting touch behind the flyout' one for you to post if you don't mind.
Depends on: 893781
Depends on: 893784
No longer depends on: 893781, 893784
Tested on 2013-07-19 as part of itereation #10 regression testing, and I encountered the problem where tapping on "Restart to Update" doesn't do anything. Leaving this as verified, but I will check bug 893781 and 893784 for when they are fixed.
(In reply to juan becerra [:juanb] from comment #21)
> Tested on 2013-07-19 as part of itereation #10 regression testing, and I
> encountered the problem where tapping on "Restart to Update" doesn't do
> anything. Leaving this as verified, but I will check bug 893781 and 893784
> for when they are fixed.

That's covered by bug 893781.
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. After updating nightly, Metro Firefox restarts.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.