1488908 - It's no longer possible to change the "mailto" handler in about:preferences

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Botond Ballo [:botond]]

STR:
  1. Load about:preferences
  2. Scroll down to the "Applications" section
  3. Click on the handle for "mailto".
     In my case, the current value is "Use mutt (default)".

Expected results:
  A dropdown opens that allows you to change the handler.

Actual results:
  Nothing happens. You can't change the handler.

This appears to be a recent regression.

Comment 1

[Botond Ballo [:botond]]

Mozregression points to bug 1484496:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b88f5fa7dca4ba5cb92d9c905f3e52d043977498&tochange=2bd127de1f05cd8379c279caea727690b847e5b9

Kris, could you have a look please?

Comment 2

[:Gijs (he/him)]

What platform are you seeing this on? Are there errors in the browser console?

(I can't reproduce on updated macOS nightly).

Comment 3

[Botond Ballo [:botond]]

I am seeing this on Linux.

There is indeed an error in the browser console, when I click on the "mailto" entry:

TypeError: handler.equals is not a function[Learn More] main.js:1751:15
rebuildActionsMenu
chrome://browser/content/preferences/in-content/main.js:1751:15
_initListEventHandlers/<
chrome://browser/content/preferences/in-content/main.js:1478:9
_fireOnSelect
chrome://global/content/bindings/richlistbox.xml:61:13
selectItem
chrome://global/content/bindings/richlistbox.xml:334:11
onxblmousedown
chrome://global/content/bindings/richlistbox.xml:1097:15

Comment 4

[Kris Maglione [:kmag]]

I don't get the `.equals()` error, but apparently this proxy:

https://searchfox.org/mozilla-central/rev/6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/uriloader/exthandler/HandlerService.js#194-219

breaks things, by not passing through set operations, and therefore ignoring our changes to the "alwaysAskBeforeHandling" property. I'm guessing the old code somehow wound up giving us a wrapped native rather than the proxy when we called QueryInterface, and the new code, by avoiding that bug, gave us a new bug.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

In the past, these operations seem to have worked by accident, by somehow
bypassing the proxy. But as things stand now, set operations set properties on
the wrapped object rather than the proxy, causing them to be mostly ignored.

Comment 6

[:Gijs (he/him)]

Comment on attachment 9007090 [details]
Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

:Gijs (he/him) has approved the revision.

Comment 7

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #5)
> Created attachment 9007090 [details]
> Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs
> 
> In the past, these operations seem to have worked by accident, by somehow
> bypassing the proxy. But as things stand now, set operations set properties
> on
> the wrapped object rather than the proxy, causing them to be mostly ignored.

I, for one, am excited to find out what bugs we're going to unearth by fixing this one...

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdd547ff76700c4a8aeaee00d645c3a9d3c893a
Bug 1488908: Add setter op to HandlerApp proxy. rs=Gijs

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/dcdd547ff767

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. Also, is this something we should have automated tests for?

Comment 11

[Kris Maglione [:kmag]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> Also, is this something we should have automated tests for?

Probably, but I don't have the time to write them.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 9007090 [details]
Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1486249
[User impact if declined]: This prevents users from changing handlers for protocols like mailto:
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a pretty trivial change that passes setter ops through a JS proxy which otherwise only handles getter ops.
[String changes made/needed]: None.

Comment 13

[Peter Magyari (Desktop QA)]

I can still reproduce this issue on Linux 18.04.1(64-bit) using Firefox Nightly 64.0a1 (20180913100107)(64-bit)

Kris, could you please take a look at this?

Comment 14

[Botond Ballo [:botond]]

Confirmed, I am still seeing the issue in the latest Nightly. The "TypeError: handler.equals is not a function" error message continues to show up in the Browser Console when the 'mailto' entry is clicked.

Comment 15

[Kris Maglione [:kmag]]

Well, someone who can reproduce this may have to look at it, then. I can't.

Comment 16

[Kris Maglione [:kmag]]

Well, someone who can reproduce this may have to look at it, then. I can't.

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9007090 [details]
Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

Not taking the uplift as there are still issues to solve, I'll be happy to take an updated patch fixing the issue.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Gijs, not necessarily asking you to do it yourself, but can you suggest someone who might be able to create tests for this bug?

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 1488908 - QI the elements of nsIGIOservice.getAppsForURIScheme() to nsIHandlerApp. r=kmag

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 9009201 [details]
Bug 1488908 - QI the elements of nsIGIOservice.getAppsForURIScheme() to nsIHandlerApp. r=kmag

Kris Maglione [:kmag] has approved the revision.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ece780edb3e8
QI the elements of nsIGIOservice.getAppsForURIScheme() to nsIHandlerApp. r=kmag

Comment 22

[Kris Maglione [:kmag]]

(In reply to Pascal Chevrel:pascalc from comment #17)
> Comment on attachment 9007090 [details]
> Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs
> 
> Not taking the uplift as there are still issues to solve, I'll be happy to
> take an updated patch fixing the issue.

We need both patches.

Comment 23

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Gijs, not necessarily asking you to do it yourself, but can you suggest
> someone who might be able to create tests for this bug?

Me, Jared or Paolo are best-placed, probably. I'll file a follow-up.

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ece780edb3e8

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 9007090 [details]
Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

resetting uplift flag since there's now a fix for the remaining issue

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9007090 [details]
Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

Both patches approved for 63 beta 7, thanks.

Comment 27

[Peter Magyari (Desktop QA)]

Verified as fixed Ubuntu 18.04 using the latest Firefox Nightly 64.0a1 (20180916220033)

Comment 28

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/03508cc9a42b
https://hg.mozilla.org/releases/mozilla-beta/rev/503cd4791845

Comment 29

[Peter Magyari (Desktop QA)]

Verified as fixed on Windows 10, Mac Os 10.12 and Ubuntu 18.04 using the latest Firefox Beta 63.0b7 (20180917143811)