Closed Bug 1488908 Opened Last year Closed Last year

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

Categories

(Firefox :: Preferences, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + verified
firefox64 + verified

People

(Reporter: botond, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
What platform are you seeing this on? Are there errors in the browser console?

(I can't reproduce on updated macOS nightly).
Flags: needinfo?(botond)
Priority: -- → P1
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
Flags: needinfo?(botond)
Depends on: 1486249
Flags: needinfo?(kmaglione+bmo)
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.
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.
Assignee: nobody → kmaglione+bmo
Comment on attachment 9007090 [details]
Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

:Gijs (he/him) has approved the revision.
Attachment #9007090 - Flags: review+
(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...
https://hg.mozilla.org/mozilla-central/rev/dcdd547ff767
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Please request Beta approval on this when you get a chance. Also, is this something we should have automated tests for?
Flags: qe-verify+
Flags: needinfo?(kmaglione+bmo)
Flags: in-testsuite?
(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.
Flags: needinfo?(kmaglione+bmo)
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.
Attachment #9007090 - Flags: approval-mozilla-beta?
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?
Flags: needinfo?(kmaglione+bmo)
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.
Well, someone who can reproduce this may have to look at it, then. I can't.
Flags: needinfo?(kmaglione+bmo)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 64 → ---
Assignee: kmaglione+bmo → nobody
Well, someone who can reproduce this may have to look at it, then. I can't.
Assignee: nobody → kmaglione+bmo
Assignee: kmaglione+bmo → botond
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.
Attachment #9007090 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Gijs, not necessarily asking you to do it yourself, but can you suggest someone who might be able to create tests for this bug?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 9009201 [details]
Bug 1488908 - QI the elements of nsIGIOservice.getAppsForURIScheme() to nsIHandlerApp. r=kmag

Kris Maglione [:kmag] has approved the revision.
Attachment #9009201 - Flags: review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ece780edb3e8
QI the elements of nsIGIOservice.getAppsForURIScheme() to nsIHandlerApp. r=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.
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1491436
https://hg.mozilla.org/mozilla-central/rev/ece780edb3e8
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
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
Attachment #9007090 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Comment on attachment 9007090 [details]
Bug 1488908: Add setter op to HandlerApp proxy. r=Gijs

Both patches approved for 63 beta 7, thanks.
Attachment #9007090 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed Ubuntu 18.04 using the latest Firefox Nightly 64.0a1 (20180916220033)
Verified as fixed on Windows 10, Mac Os 10.12 and Ubuntu 18.04 using the latest Firefox Beta 63.0b7 (20180917143811)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.