Cristian Baica [:cbaica], Release Desktop QA Reporter
	Description • 6 years ago

[Affected versions]:
- Fx63.0b9

[Affected platforms]:
- Window 7 x64

[Steps to reproduce]:
1. Run the Firefox installer, making sure the option to make Firefox the default browser is checked.
2. Uninstall Firefox from the control panel.
3. After the uninstall is finished, check the option to "Tell Mozilla why you uninstalled Firefox".

[Expected result]:
- The user should have the uninstall survey web-page displayed in another browser available on the system (e.g. IE).

[Actual result]:
- An error is received informing the user that there is no program associated with the type of file he wants to open.

[Regression range]:
- This is not a new regression since it's related to a new feature.

[Additional notes]:
- In the "Default Programs" section, IE appears as the default program to open .html files.
- This seems to be isolated to my machine, because on another try on a different PC(with windows 7 setup), the issue couldn't be reproduced.
- The issue is not reproducible on windows 8.1 or 10.

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 2 • 6 years ago

Tracking for 63 as per the Beta Preliminary status report from QA which identified fixing and uplifting this bug as important for the readiness of the feature for release.

	Molly Howell (she/her) (no longer active) Assignee
	Comment 3 • 6 years ago

I've not been able to reproduce this at all, but I think I can fix it by forcing the page to open in Internet Explorer instead of trying to use the default browser setting.

	Adam Gashlin (he/him) [:agashlin] (ex-moco)
	Comment 6 • 6 years ago

> +    if (args[0] != L'\0') {
> +      pushstring(stacktop, args, lstrlenW(args));
> +    }
Is it not possible for there to have been an empty string on the stack?

> +      ExecInExplorer::Exec "C:\Program Files\Internet Explorer\iexplore.exe" /cmdargs "$R1"
I'm not sure how this works with localization, is this path going to work on all languages and reasonable versions of IE?

	Molly Howell (she/her) (no longer active) Assignee
	Comment 7 • 6 years ago

(In reply to Adam Gashlin [:agashlin] from comment #6)
> > +    if (args[0] != L'\0') {
> > +      pushstring(stacktop, args, lstrlenW(args));
> > +    }
> Is it not possible for there to have been an empty string on the stack?

Yeah, I didn't think of that. This should check for the stack itself actually being empty.

> > +      ExecInExplorer::Exec "C:\Program Files\Internet Explorer\iexplore.exe" /cmdargs "$R1"
> I'm not sure how this works with localization, is this path going to work on
> all languages and reasonable versions of IE?

Oh, geez, yeah. At the very least I shouldn't have hard coded "Program Files". I'll look into this.

	Molly Howell (she/her) (no longer active) Assignee
	Comment 8 • 6 years ago

Improved the stack restoration logic.

	Molly Howell (she/her) (no longer active) Assignee
	Comment 9 • 6 years ago

Turns out you don't even need the path to iexplore.exe, presumably because it's registered in App Paths.

	Pulsebot
	Comment 10 • 6 years ago

Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6006b0096da2
Part 1 - Support optional command line arguments in the ExecInExplorer plugin. r=agashlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d8c5328915
Part 2 - Always use IE to show the uninstall survey on Win7/8. r=agashlin

	Arthur Iakab [arthur_iakab]
	Comment 11 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6006b0096da2
https://hg.mozilla.org/mozilla-central/rev/b8d8c5328915

	Cristian Baica [:cbaica], Release Desktop QA Reporter
	Comment 12 • 6 years ago

Tested on the latest Fx 64.0a1. The survey is opened using IE.

Waiting for the uplift to beta in order to close the ticket.

	Molly Howell (she/her) (no longer active) Assignee
	Comment 13 • 6 years ago

We've got a regression that this patch created (bug 1495212); I need to land a fix for that before anything can get uplifted.

	Molly Howell (she/her) (no longer active) Assignee
	Comment 14 • 6 years ago

Comment on attachment 9012981 [details] [diff] [review]
Part 1 - Support optional command line arguments in the ExecInExplorer plugin

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1448804

User impact if declined: Uninstall survey may not open on Windows 7 and 8 (Windows 10 isn't affected).

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1495212

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch has been verified by QA (see comment 12).
Also, the affected feature is low impact and self-contained and the code runs after the end of the uninstallation, so the uninstall process itself is not impacted.

String changes made/needed:

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago

Comment on attachment 9012981 [details] [diff] [review]
Part 1 - Support optional command line arguments in the ExecInExplorer plugin

Tracked bug for 63, approved for 63.0b13.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/a48d5341b695
https://hg.mozilla.org/releases/mozilla-beta/rev/16e9bc01eb3d

	Cristian Baica [:cbaica], Release Desktop QA Reporter
	Comment 17 • 6 years ago

Verified as fixed on latest beta (Fx63.0b13) and latest nightly. 
The uninstall survey is now correctly opened in IE after having Firefox set as the default browser. If another browser is set as default (e.g. Chrome) the uninstall survey page will still open in IE as per implementation.