1722758 - Mac Share menu is empty for URLs with double # because getSharingProviders throws exceptions

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Haik Aftandilian [:haik]]

Attachment: Screenshot demonstrating the problem

For some URLs, the File->Share menu (and also the tab right-click -> Share menu) is empty. See screenshot for example.

The problem occurs with tabs that have the chat.mozilla.org site open such as

https://chat.mozilla.org/#/room/#macdev:mozilla.org
https://chat.mozilla.org/#/room/#crashreporting:mozilla.org

I encountered this on on macOS 11.5, running Nightly 92.0a1 (2021-07-28). :mstange also reproduced the problem.

Comment 1

[WIlliam Davis [:AwesomeSheep48]]

Happens on my self-hosted element client too

Comment 2

[WIlliam Davis [:AwesomeSheep48]]

I don't know how I should format the mozregression output, so here are the last few lines of it

2021-07-28T17:27:55.524000: INFO : Narrowed integration regression window from [acf8c90e, 1ecacaba] (3 builds) to [093bb7a6, 1ecacaba] (2 builds) (~1 steps left)
2021-07-28T17:27:55.545000: DEBUG : Starting merge handling...
2021-07-28T17:27:55.545000: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=1ecacabac7285f44c99499300656f5c022022866&full=1
2021-07-28T17:27:55.546000: DEBUG : redo: attempt 1/3
2021-07-28T17:27:55.546000: DEBUG : redo: retry: calling _default_get with args: ('https://hg.mozilla.org/integration/autoland/json-pushes?changeset=1ecacabac7285f44c99499300656f5c022022866&full=1',), kwargs: {}, attempt #1
2021-07-28T17:27:55.577000: DEBUG : urllib3.connectionpool: Resetting dropped connection: hg.mozilla.org
2021-07-28T17:27:56.810000: DEBUG : urllib3.connectionpool: https://hg.mozilla.org:443 "GET /integration/autoland/json-pushes?changeset=1ecacabac7285f44c99499300656f5c022022866&full=1 HTTP/1.1" 200 None
2021-07-28T17:27:56.863000: DEBUG : Found commit message:
Bug 1512851 - make share menu handling more generic and replace the file menu's email link with it on macOS, r=sfoster,fluent-reviewers,flod

Depends on D120637

Differential Revision: https://phabricator.services.mozilla.com/D120638

2021-07-28T17:27:56.863000: DEBUG : Did not find a branch, checking all integration branches
2021-07-28T17:27:56.875000: INFO : The bisection is done.
2021-07-28T17:27:56.880000: INFO : Stopped

Comment 3

[Haik Aftandilian [:haik]]

Thanks, William.

Gijs, could you take a look?

Comment 4

[:Gijs (he/him)]

As I like to keep saying, "post hoc ergo propter hoc" is a hell of a drug.

bug 1512851 implemented the File menu support; before that there was no Share entry in the file menu at all. However, there was a "Share" submenu of the tab context menu which existed before 1512851, and it already didn't work in release and older builds on similar URLs. Before that, there was a "Share" entry in the meatball menu in the address bar, and although I haven't bothered to check, I'm like 99% confident that that also would have been broken.

The problem is that calling getSharingProviders with such URLs https://chat.mozilla.org/#/room/#macdev:mozilla.org throws a JS exception (NS_ERROR_FAILURE). The cocoa APIs we provide shouldn't do that, and I don't know why it does. Maybe macOS can't cope with the double # when constructing NSURL or something?

We could work around in the frontend but showing a populated menu that then won't work when we actually send the URL to any of the apps is arguably worse...

Comment 5

[Haik Aftandilian [:haik]]

We use [NSURL URLWithString:] for this and Apple's docs state that the argument "Must be a URL that conforms to RFC 2396. This method parses URLString according to RFCs 1738 and 1808."

And RFC 1738 states that

All unsafe characters must always be encoded within a URL. For
example, the character "#" must be encoded within URLs even in
systems that do not normally deal with fragment or anchor
identifiers, so that if the URL is copied into another system that
does use them, it will not be necessary to change the URL encoding.

It's likely we need to encode # and other characters before passing the string to [NSURL URLWithString:].

Comment 6

[Haik Aftandilian [:haik]]

Attachment: Bug 1722758 - Patch 1 - Reformat the encoding table r?#necko-reviewers

Rearrange and reformat the encoding table to make space for two digit entries needed for an additional encoding enum.

Convert entries to hex.

Comment 7

[Haik Aftandilian [:haik]]

Attachment: Bug 1722758 - Patch 2 - Add a new encoding mode to be used for Apple NSURL compatibility r?#necko-reviewers

Adds a new encoding mode to be used to encode an already-encoded URL to be compatible with Apple's NSURL.

Depends on D122650

Comment 8

[Haik Aftandilian [:haik]]

Attachment: Bug 1722758 - Patch 3 - Change netCharType to be an unsigned char array instead of int to save space r?#necko-reviewers

Depends on D122651

Comment 9

[Haik Aftandilian [:haik]]

Attachment: Bug 1722758 - Patch 4 - Encode additional characters in the URL required for NSURL compatiblity r?#mac-reviewers

Depends on D122652

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6eda5c14afa
Patch 1 - Reformat the encoding table r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/d6fb40ffa719
Patch 2 - Add a new encoding mode to be used for Apple NSURL compatibility r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/3bcb1a3c8320
Patch 3 - Change netCharType to be an unsigned char array instead of int to save space r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/e7ae5590cd1b
Patch 4 - Encode additional characters in the URL required for NSURL compatiblity r=mac-reviewers,mstange

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a6eda5c14afa
https://hg.mozilla.org/mozilla-central/rev/d6fb40ffa719
https://hg.mozilla.org/mozilla-central/rev/3bcb1a3c8320
https://hg.mozilla.org/mozilla-central/rev/e7ae5590cd1b

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:haik, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Simona Badau, Desktop QA]

Reproduced on Firefox 92 beta 7.

Verified as fixed on the latest Nightly 93.0a1 (20210823092315) on Mac OS X 10.15.7 and macOS Big Sur 11.5.2.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Too late for Fx92.