Both release and nightly claims they are default browser

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Installer
--
minor
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: atlanto, Assigned: mhowell)

Tracking

({regression})

54 Branch
Firefox 54
x86_64
Windows 10
regression
Points:
---

Firefox Tracking Flags

(firefox51 unaffected, firefox52 unaffected, firefox53 unaffected, firefox54+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
I have Firefox(Release) and Nightly installations and Release is my default browser, Release shows "Firefox is currently your default browser" at Options>General>Startup.
Now, Nightly also is showing "Nightly is currently your default browser".

mozregression :(
 4:16.07 INFO: Last good revision: 25a94c1047e793ef096d8556fa3c26dd72bd37d7
 4:16.09 INFO: First bad revision: b83e2b2524c981eabae7c48f8ea6988544e5087a
 4:16.09 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=25a94c1047e793ef096d8556fa3c26dd72bd37d7&tochange=b83e2b2524c981eabae7c48f8ea6988544e5087a
 4:17.43 ERROR: The url u'https://hg.mozilla.org/integration/fx-team/json-pushes?changeset=b83e2b2524c981eabae7c48f8ea6988544e5087a&full=1' returned a 404 error. Please check the validity of the url.

maybe Bug 1337530 ?

Comment 1

4 months ago

Regression confirmed.
Blocks: 1337530
Status: UNCONFIRMED → NEW
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → affected
tracking-firefox54: --- → ?
Ever confirmed: true
Flags: needinfo?(mhowell)
Keywords: regression
nsWindowsShellService::IsDefaultBrowser used to check whether the file path in the registry points self. Bug 1337530 removed the check. Bug 1337530 removed the old code a bit too aggressively. The registry check should be restored.
(Assignee)

Comment 3

4 months ago
:emk is correct; I'll restore the path check.
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Flags: needinfo?(mhowell)
Tracking 53+ for this regression.
tracking-firefox54: ? → +
Comment hidden (mozreview-request)
(Assignee)

Comment 6

4 months ago
Comment on attachment 8836927 [details]
Bug 1338843 - Use the install path to distinguish between multiple installations when checking default browser status.

I manually tested this patch by the following procedure:
1) Install the current Nightly and make it the default browser.
2) Install a custom build with this patch. Observe that this build does not report being the default browser.
3) Make the custom build the default browser. Observe that it now does show itself as the default browser. (the official Nightly build does as well, because of this bug)

Comment 7

4 months ago
mozreview-review
Comment on attachment 8836927 [details]
Bug 1338843 - Use the install path to distinguish between multiple installations when checking default browser status.

https://reviewboard.mozilla.org/r/112242/#review113520

r- because of buffer overrun bugs.

But am I eligible to review in the first place? I'm not a peer of this module.

::: browser/components/shell/nsWindowsShellService.cpp:250
(Diff revision 1)
> +    if (REG_FAILED(res)) {
> +      return false;
> +    }
> +
> +    wchar_t exePath[MAX_BUF] = L"";
> +    if (!::GetModuleFileNameW(0, exePath, sizeof(exePath))) {

GetModuleFileNameW will take the buffer length in wide characters. But sizeof(exePath) == MAX_BUF * sizeof(wchar_t). So this will cause buffer overrun.

Use ArrayLength(exePath) or MAX_BUF.

::: browser/components/shell/nsWindowsShellService.cpp:255
(Diff revision 1)
> +    if (!::GetModuleFileNameW(0, exePath, sizeof(exePath))) {
> +      return false;
> +    }
> +    // Convert the path to a long path since GetModuleFileNameW returns the path
> +    // that was used to launch Firefox which is not necessarily a long path.
> +    if (!::GetLongPathNameW(exePath, exePath, sizeof(exePath))) {

Same as above. Use ArrayLength or MAX_BUF.

::: browser/components/shell/nsWindowsShellService.cpp:260
(Diff revision 1)
> +    if (!::GetLongPathNameW(exePath, exePath, sizeof(exePath))) {
> +      return false;
> +    }
> +
> +    wchar_t fullCmd[MAX_BUF] = L"";
> +    _snwprintf(fullCmd, sizeof(fullCmd), L"\"%s\" -osint -url \"%%1\"", exePath);

And here.

(sizeof(cmdFromReg) is OK because RegQueryValueExW will take the buffer length in bytes.)
Attachment #8836927 - Flags: review?(VYV03354) → review-
(Assignee)

Comment 8

4 months ago
mozreview-review-reply
Comment on attachment 8836927 [details]
Bug 1338843 - Use the install path to distinguish between multiple installations when checking default browser status.

https://reviewboard.mozilla.org/r/112242/#review113520

Thanks, revised patch coming up.

You've been providing really good feedback around this work and in this file specifically, so I just assumed you were qualified. Being a formal module peer isn't really required, just strong familiarity with the material. If you don't feel comfortable with that, then I can certainly move the request.
Comment hidden (mozreview-request)

Comment 10

4 months ago
mozreview-review
Comment on attachment 8836927 [details]
Bug 1338843 - Use the install path to distinguish between multiple installations when checking default browser status.

https://reviewboard.mozilla.org/r/112242/#review113930

OK, thank you. Looks good to me.

::: browser/components/shell/nsWindowsShellService.cpp:212
(Diff revision 2)
>  
>    return LaunchHelper(appHelperPath);
>  }
>  
>  static bool
>  IsAARDefault(const RefPtr<IApplicationAssociationRegistration>& pAAR,

nit: It would be better to rename this function because it is doing more checks than IAAR call now.
Attachment #8836927 - Flags: review?(VYV03354) → review+

Comment 11

4 months ago
mozreview-review
Comment on attachment 8836927 [details]
Bug 1338843 - Use the install path to distinguish between multiple installations when checking default browser status.

https://reviewboard.mozilla.org/r/112242/#review113936

I forgot one more comment.

::: browser/components/shell/nsWindowsShellService.cpp:249
(Diff revision 2)
> +    ::RegCloseKey(theKey);
> +    if (REG_FAILED(res)) {
> +      return false;
> +    }
> +
> +    wchar_t exePath[MAX_BUF] = L"";

IMO GetModuleFileNameW and GetLongPathNameW should be called outside this function and exePath should be passed as a parameter because exePath does not depend on aClassName and GetLongPathNameW requires disk access.
Comment hidden (mozreview-request)

Comment 13

4 months ago
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba64203b54e1
Use the install path to distinguish between multiple installations when checking default browser status. r=emk

Comment 14

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba64203b54e1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
I have reproduce this bug with Nightly 54.0a1 (2017-02-11) and Release 53 (64- bit) in Windows 10.

This bug's fix is verified with latest Beta 54.0b3 (64-bit).
 
Build ID   :	20170427091925
User Agent :	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0


[testday-20170428]
You need to log in before you can comment on or make changes to this bug.