[MP] Defect - Manual or disabled updates from Metro needs cleanup

RESOLVED FIXED in Firefox 26

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
Firefox 26
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 3 obsolete attachments)

Assignee

Description

6 years ago
There are some problems with manual and disabled updates when you try to upgrade through Metro.
1) The browser needs to be manually restarted to finish the update
2) The browser lags when clicking restart for a few seconds
3) The update.status == succeeded files doesn't get cleaned  
   (For some reason code doesn't get called when app.update.metro.enabled = false)

These 3 problems are not present for Metro updates when Metro updates are enabled.
These problems are not present for Desktop updates.

If upgrading from Metro with the service disabled, #1 applies as well.
update.status == pending if you go and look at it manually when service is disabled after restarting the app.


p=3
Assignee

Updated

6 years ago
Summary: Manual or disabled updates from Metro needs cleanup → Defect - Manual or disabled updates from Metro needs cleanup
Assignee

Updated

6 years ago
No longer blocks: 794937
Assignee

Updated

6 years ago
Blocks: 833182
Assignee

Updated

6 years ago
Priority: -- → P3
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0
Assignee

Updated

6 years ago
Priority: P3 → P2
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 [preview]
Summary: Defect - Manual or disabled updates from Metro needs cleanup → MP Defect - Manual or disabled updates from Metro needs cleanup
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 [preview] → [preview] feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0
Summary: MP Defect - Manual or disabled updates from Metro needs cleanup → [MP] Defect - Manual or disabled updates from Metro needs cleanup
Blocks: metrov1backlog
No longer blocks: metrov1defect&change
Assignee

Updated

6 years ago
Blocks: metrov1it14
Status: NEW → ASSIGNED
QA Contact: netzen
Whiteboard: [preview] feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 → [preview] feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=3
Assignee: nobody → netzen
No longer blocks: metrov1backlog
QA Contact: netzen → jbecerra
Assignee

Comment 1

6 years ago
It seems like the launched updater.exe process is under control by Windows using Metro app rules. Windows then kills the process for waiting too long.

The program updater.exe version 26.0.0.4986 stopped interacting with Windows and was closed. To see if more information about the problem is available, check the problem history in the Action Center control panel.
 Process ID: 4f0
 Start Time: 01cea3ff547c334d
 Termination Time: 4294967295
 Application Path: C:\Users\Brian\AppData\Local\Mozilla\updates\EEFEA8717BC47F65\updates\0\updater.exe
 Report Id: 9ba3b866-0ff2-11e3-bebb-da35ce4b9f9a
 Faulting package full name: 
 Faulting package-relative application ID:
Assignee

Comment 6

6 years ago
Tested this on oak and it successfully restarts the browser even when the service is not in use.
Comment on attachment 799537 [details] [diff] [review]
Patch 1 rev 1 - Add support to CEH to restart Metro Firefox when it isn't running.

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

::: browser/metro/shell/commandexecutehandler/CEHHelper.cpp
@@ +69,5 @@
> +  typedef BOOL (WINAPI* IsImmersiveProcessFunc)(HANDLE process);
> +  IsImmersiveProcessFunc IsImmersiveProcessPtr =
> +    (IsImmersiveProcessFunc)GetProcAddress(user32DLL,
> +                                           "IsImmersiveProcess");
> +  FreeLibrary(user32DLL);

nit - should be below the call to IsImmersiveProcessPtr.

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +658,5 @@
> +  // until the previous instance closes.  We don't want to wait too long
> +  // because the browser could appear to randomly start for the user. We want
> +  // it to also be long enough so the browser has time to close.
> +  const size_t kWaitPerRetry = 50;
> +  const size_t kMaxWaitTime = 38000;

nit - lets move these plus the comment up top with REQUEST_WAIT_TIMEOUT.
Attachment #799537 - Flags: review?(jmathies) → review+
Comment on attachment 799538 [details] [diff] [review]
Patch 2 rev 1 - Respect app restart requests in Metro Firefox

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

::: widget/windows/winrt/MetroAppShell.cpp
@@ +90,5 @@
>    return nsBaseAppShell::Init();
>  }
>  
> +HRESULT
> +SHCreateItemFromParsingNameDynamic(PCWSTR pszPath, IBindCtx *pbc, REFIID riid, void **ppv)

You should be able to use the WinUtils version here - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.h#225

@@ +134,5 @@
> +
> +BOOL
> +WinLaunchDeferredMetroFirefox()
> +{
> +  CoInitialize(NULL);

nit - do we really need this? If so, lets match it up with an uninit.

@@ +217,5 @@
> +
> +      nsCOMPtr<nsIAppStartup> appStartup (do_GetService(NS_APPSTARTUP_CONTRACTID));
> +      bool restarting;
> +      if (appStartup && NS_SUCCEEDED(appStartup->GetRestarting(&restarting)) && restarting) {
> +        WinLaunchDeferredMetroFirefox();

Since we go through all the trouble of returning a result here, lets check it and dump something to the Log when it fails.
Attachment #799538 - Flags: review?(jmathies) → review+

Updated

6 years ago
Attachment #799540 - Flags: review?(jmathies) → review+

Updated

6 years ago
Attachment #799541 - Flags: review?(jmathies) → review+
Assignee

Comment 9

6 years ago
Implemented review comments, carrying forward r+
Attachment #799537 - Attachment is obsolete: true
Attachment #799614 - Flags: review+
Assignee

Comment 10

6 years ago
Implemented review comments, carrying forward r+.
Attachment #799538 - Attachment is obsolete: true
Attachment #799621 - Flags: review+
Assignee

Comment 11

6 years ago
Please see the description within the patch for more context.  The problem this fixes is that even know the update.status contained succeeded, the Metro flyout panel, which has the same code as the about dialog, would show "Restart to Update".  Recall that Metro updates are considered disabled for all options except for the top most "Apply updates from both Desktop and Metro Firefox" option in preferences.
Attachment #801176 - Flags: review?(robert.bugzilla)
Comment on attachment 801176 [details] [diff] [review]
Patch 5 rev 1 - Restart to update sticks when updates from Metro are disabled


>@@ -2069,17 +2069,20 @@ UpdateService.prototype = {
> 
>   /**
>    * Perform post-processing on updates lingering in the updates directory
>    * from a previous application session - either report install failures (and
>    * optionally attempt to fetch a different version if appropriate) or
>    * notify the user of install success.
>    */
>   _postUpdateProcessing: function AUS__postUpdateProcessing() {
>-    if (!this.canCheckForUpdates) {
>+    // canCheckForUpdates will return false when metro-only updates are disabled
>+    // from within metro.  In that case we still want _postUpdateProcessing to
>+    // run.  gMetroUpdatesEnabled returns true on non Windows 8 platforms.
>+    if (!this.canCheckForUpdates && gMetroUpdatesEnabled) {
Please expand to include why we still want _postUpdateProcessing to run
Attachment #801176 - Flags: review?(robert.bugzilla) → review+
Assignee

Comment 13

6 years ago
Implemented review comment, carrying forward r+.
Attachment #801176 - Attachment is obsolete: true
Attachment #801759 - Flags: review+
Assignee

Comment 14

6 years ago
Manually tested these patches along with bug 893784 w/ these manual steps and everything passed without issues:

- desktop update w/ desktop + win8 option & service enabled
- desktop update w/ desktop only option & service enabled
- desktop update w/ check but let me choose when to install update option & service enabled
- desktop update w/ disabled update option & service enabled
- metro update w/ desktop + win8 option & service enabled
- metro update w/ desktop only option & service enabled
- metro update w/ check but let me choose when to install update option & service enabled
- metro update w/ disabled update option & service enabled
- desktop update w/ desktop + win8 option & service disabled
- desktop update w/ desktop only option & service disabled
- desktop update w/ check but let me choose when to install update option & service disabled
- desktop update w/ disabled update option & service disabled
- metro update w/ desktop + win8 option & service disabled
- metro update w/ desktop only option & service disabled
- metro update w/ check but let me choose when to install update option & service disabled
- metro update w/ disabled update option & service disabled
- desktop update while metro fx is open & service enabled
- metro update while desktop fx is open & service enabled
- desktop update while metro fx is open & service disabled
- metro update while desktop fx is open & service disabled
- desktop update w/ service when staging is disabled
- metro update w/ service when staging is disabled
Why the direct mozilla-central landing (as opposed to fx-team), out of curiosity? :-)
Flags: needinfo?(netzen)
Assignee

Comment 17

6 years ago
(In reply to Ed Morley [:edmorley UTC+1] from comment #16)
> Why the direct mozilla-central landing (as opposed to fx-team), out of
> curiosity? :-)

Is that not allowed anymore? I've done this in the past for dozens of update related bugs.

I have a couple reasons:
1) I want to make sure it is landed on m-c before tonight so I can test updates tomorrow morning first thing. I want to deterministically predict when it hits m-c for testing reasons.

2) We are at our Metro iteration's end today, and I wanted this bug to be completed so it is included in that iteration.
Flags: needinfo?(netzen)
Assignee

Updated

6 years ago
Flags: needinfo?(emorley)
(In reply to Brian R. Bondy [:bbondy] from comment #17)
> Is that not allowed anymore? I've done this in the past for dozens of update
> related bugs.

It's informally discouraged now that we have more sheriff-managed integration repos that merge normally twice a day each into mozilla-central. No worries for this time, since there isn't a formal policy yet (we'll soon be drafting one) and from the sounds of this may have been a worthy example of the exceptions that such a policy will cater for. Was mainly just curious for now :-)
Flags: needinfo?(emorley)
You need to log in before you can comment on or make changes to this bug.