Closed Bug 1338843 Opened 7 years ago Closed 7 years ago

Both release and nightly claims they are default browser

Categories

(Firefox :: Installer, defect)

Product:
Firefox
Component:
Installer
Version:
54 Branch
Platform:
x86_64
Windows 10
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 + fixed

People

(Reporter: euthanasia_waltz, Assigned: molly)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1338843 - Use the install path to distinguish between multiple installations when checking default browser status.
59 bytes, text/x-review-board-request
emk
: review+
Details 
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 ?

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.
: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 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 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-
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 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 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.
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
https://hg.mozilla.org/mozilla-central/rev/ba64203b54e1
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: