Closed
Bug 950241
Opened 11 years ago
Closed 11 years ago
Takes two tries to start up Firefox in Touch mode
Categories
(Firefox for Metro Graveyard :: General, defect, P2)
Tracking
(firefox28 verified, firefox29 verified, b2g-v1.3 fixed)
VERIFIED
FIXED
Firefox 29
People
(Reporter: krudnitski, Assigned: jimm)
References
Details
(Whiteboard: [beta28] [defect] p=2 [landing: 02-01 nightly; verifiable in 02-02 nightly with 02-01 nightly installed])
Attachments
(10 files, 9 obsolete files)
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jimm
:
review+
robert.strong.bugs
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
(don't know how this landed as a security-sensitive bug - that was certainly not my intention, nor am I sure how that happened!)
Reporter | ||
Comment 2•11 years ago
|
||
Therefore please uncheck this bug if you could so the metro team can figure out what's happening in this bug :)
Updated•11 years ago
|
Group: core-security
Comment 3•11 years ago
|
||
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.
Updated•11 years ago
|
Blocks: metrov1backlog
Assignee | ||
Comment 4•11 years ago
|
||
I see this fairly often when there's an update pending.
Reporter | ||
Comment 5•11 years ago
|
||
it's been happening every time on my SP2 for the past week, except yesterday. Yesterday it started on the first tap.
Updated•11 years ago
|
Whiteboard: [triage] → [beta28]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jmathies
Whiteboard: [beta28] → [beta28] p=2
Updated•11 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Assignee | ||
Comment 6•11 years ago
|
||
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.
Attachment #8348659 -
Flags: review?(netzen)
Assignee | ||
Updated•11 years ago
|
Attachment #8348659 -
Attachment description: remove no splash option → debug patch
Attachment #8348659 -
Flags: review?(netzen)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [beta28] p=2 → [leave-open][beta28] p=2
Comment 8•11 years ago
|
||
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.
Attachment #8348662 -
Flags: review?(netzen) → review+
Assignee | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
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?)
Flags: needinfo?(netzen)
Comment 13•11 years ago
|
||
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.
Flags: needinfo?(netzen)
Comment 14•11 years ago
|
||
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
Assignee | ||
Comment 15•11 years ago
|
||
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
Attachment #8348662 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #8348662 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•11 years ago
|
||
status-firefox28:
--- → fixed
status-firefox29:
--- → fixed
Assignee | ||
Comment 17•11 years ago
|
||
> 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.
Assignee | ||
Comment 18•11 years ago
|
||
(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•11 years ago
|
||
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
Updated•11 years ago
|
Assignee | ||
Comment 20•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [leave-open][beta28] p=2 → [leave-open][beta28] [defect] p=2
Updated•11 years ago
|
Updated•11 years ago
|
Assignee | ||
Comment 21•11 years ago
|
||
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Comment 23•11 years ago
|
||
Assignee | ||
Comment 24•11 years ago
|
||
Now that we restart after xpcom shutdown I don't think we need this anymore.
Assignee | ||
Comment 25•11 years ago
|
||
Assignee | ||
Comment 26•11 years ago
|
||
Assignee | ||
Comment 27•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave-open][beta28] [defect] p=2 → [beta28] [defect] p=2
Updated•11 years ago
|
Attachment #8362976 -
Flags: feedback+
Updated•11 years ago
|
Attachment #8362978 -
Flags: feedback+
Updated•11 years ago
|
Attachment #8362992 -
Flags: feedback+
Comment 29•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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?
Assignee | ||
Comment 32•11 years ago
|
||
(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.
Assignee | ||
Comment 33•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 36•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
status-firefox28:
fixed → ---
status-firefox29:
fixed → ---
Assignee | ||
Comment 38•11 years ago
|
||
Attachment #8348659 -
Attachment is obsolete: true
Attachment #8348662 -
Attachment is obsolete: true
Attachment #8362974 -
Attachment is obsolete: true
Assignee | ||
Comment 39•11 years ago
|
||
Attachment #8362976 -
Attachment is obsolete: true
Assignee | ||
Comment 40•11 years ago
|
||
Attachment #8362978 -
Attachment is obsolete: true
Assignee | ||
Comment 41•11 years ago
|
||
Attachment #8362979 -
Attachment is obsolete: true
Assignee | ||
Comment 42•11 years ago
|
||
Attachment #8362981 -
Attachment is obsolete: true
Assignee | ||
Comment 43•11 years ago
|
||
Attachment #8362992 -
Attachment is obsolete: true
Assignee | ||
Comment 44•11 years ago
|
||
Assignee | ||
Comment 45•11 years ago
|
||
Attachment #8363003 -
Attachment is obsolete: true
Assignee | ||
Comment 46•11 years ago
|
||
Assignee | ||
Comment 47•11 years ago
|
||
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.
Attachment #8366663 -
Flags: feedback?(robert.bugzilla)
Assignee | ||
Comment 48•11 years ago
|
||
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?
Flags: needinfo?(netzen)
Comment 49•11 years ago
|
||
(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.
Flags: needinfo?(netzen)
Assignee | ||
Comment 50•11 years ago
|
||
(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•11 years ago
|
||
> 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
Assignee | ||
Comment 52•11 years ago
|
||
(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.
Assignee | ||
Comment 53•11 years ago
|
||
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•11 years ago
|
||
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.
Attachment #8366663 -
Flags: feedback?(robert.bugzilla) → feedback+
Comment 55•11 years ago
|
||
You can r=me with the commenting out
Assignee | ||
Comment 56•11 years ago
|
||
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.
Attachment #8366654 -
Flags: review?(netzen)
Assignee | ||
Comment 57•11 years ago
|
||
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.
Attachment #8366655 -
Flags: review?(netzen)
Assignee | ||
Comment 58•11 years ago
|
||
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.
Attachment #8366659 -
Flags: review?(netzen)
Assignee | ||
Comment 59•11 years ago
|
||
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.
Attachment #8366660 -
Flags: review?(netzen)
Assignee | ||
Comment 60•11 years ago
|
||
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.
Attachment #8366661 -
Flags: review?(netzen)
Assignee | ||
Comment 61•11 years ago
|
||
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.
Attachment #8366662 -
Flags: review?(robert.bugzilla)
Assignee | ||
Updated•11 years ago
|
Attachment #8366663 -
Flags: review+
Assignee | ||
Comment 62•11 years ago
|
||
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.
Attachment #8366664 -
Flags: review?(netzen)
Comment 63•11 years ago
|
||
Comment on attachment 8366662 [details] [diff] [review]
part 6 - update nsUpdateDriver restart check
Brian, can you take this?
Attachment #8366662 -
Flags: review?(robert.bugzilla) → review?(netzen)
Assignee | ||
Comment 64•11 years ago
|
||
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.
Attachment #8366665 -
Flags: review?(netzen)
Assignee | ||
Comment 65•11 years ago
|
||
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.
Attachment #8366745 -
Flags: review?(netzen)
Updated•11 years ago
|
Attachment #8366654 -
Flags: review?(netzen) → review+
Updated•11 years ago
|
Attachment #8366655 -
Flags: review?(netzen) → review+
Updated•11 years ago
|
Attachment #8366659 -
Flags: review?(netzen) → review+
Updated•11 years ago
|
Attachment #8366661 -
Flags: review?(netzen) → review+
Comment 66•11 years ago
|
||
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.
Attachment #8366662 -
Flags: review?(netzen) → review+
Updated•11 years ago
|
Attachment #8366664 -
Flags: review?(netzen) → review+
Updated•11 years ago
|
Attachment #8366660 -
Flags: review?(netzen) → review+
Comment 67•11 years ago
|
||
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.
Attachment #8366665 -
Flags: review?(netzen) → review+
Comment 68•11 years ago
|
||
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.
Attachment #8366745 -
Flags: review?(netzen)
Assignee | ||
Comment 69•11 years ago
|
||
Updated per comments and pushed to oak.
Assignee | ||
Comment 70•11 years ago
|
||
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•11 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Assignee | ||
Updated•11 years ago
|
Whiteboard: [beta28] [defect] p=2 → [beta28] [defect] p=2 [landing: 02-01 nightly; verifyable in 02-02 nightly with 02-01 nightly installed]
Comment 72•11 years ago
|
||
Kamil, can you please test to confirm this is resolved?
status-firefox29:
--- → fixed
Flags: needinfo?(kamiljoz)
Comment 73•11 years ago
|
||
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•11 years ago
|
||
This is using Fx29 Desktop version on Windows 8.1 Pro x64.
Comment 75•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
Nightly http://hg.mozilla.org/mozilla-central/rev/735a648bca0d does the same thing except it produces one less error message.
Comment 78•11 years ago
|
||
I believe this was a respin of Nightly.
Comment 79•11 years ago
|
||
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•11 years ago
|
||
Thanks Gary.
Kamil, can you please test this to confirm what Gary is seeing?
Comment 81•11 years ago
|
||
Let me add that I tested with a new profile.
Assignee | ||
Comment 82•11 years ago
|
||
(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)?
Assignee | ||
Updated•11 years ago
|
Whiteboard: [beta28] [defect] p=2 [landing: 02-01 nightly; verifyable in 02-02 nightly with 02-01 nightly installed] → [beta28] [defect] p=2 [landing: 02-01 nightly; verifiable in 02-02 nightly with 02-01 nightly installed]
Assignee | ||
Comment 83•11 years ago
|
||
Due to bug 966626, I've cancelled the 02-01 Windows nightly.
Whiteboard: [beta28] [defect] p=2 [landing: 02-01 nightly; verifiable in 02-02 nightly with 02-01 nightly installed] → [beta28] [defect] p=2 [landing: 02-02 nightly; verifiable in 02-03 nightly with 02-02 nightly installed]
Assignee | ||
Comment 84•11 years ago
|
||
I retriggered nightlies for this.
Whiteboard: [beta28] [defect] p=2 [landing: 02-02 nightly; verifiable in 02-03 nightly with 02-02 nightly installed] → [beta28] [defect] p=2 [landing: 02-01 nightly; verifiable in 02-02 nightly with 02-01 nightly installed]
Comment 85•11 years ago
|
||
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!
Flags: needinfo?(kamiljoz)
Comment 86•11 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•11 years ago
|
status-firefox28:
--- → affected
Assignee | ||
Comment 87•11 years ago
|
||
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.
Attachment #8366745 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #8366745 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 88•11 years ago
|
||
(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+?
Flags: needinfo?(jorge)
Assignee | ||
Comment 89•11 years ago
|
||
(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.
Assignee | ||
Comment 90•11 years ago
|
||
Note, bug 966626 must get uplifted with this patch set.
Comment 91•11 years ago
|
||
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
Flags: needinfo?(jorge)
Comment 92•11 years ago
|
||
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
Updated•11 years ago
|
status-b2g-v1.3:
--- → fixed
Comment 93•11 years ago
|
||
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).
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•