1498934 - Opening magnet links no longer works

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P1)

Description

[Marco Castelluccio [:marco]]

Similarly to bug 1498932, caused by either bug 1484496 or bug 1484373.

In this case, I can no longer press on "Open Link", the option is greyed out.

I see "TypeError: handler.equals is not a function" in the Browser Console.

I can only reproduce this in my profile, not in a clean one.

Comment 1

[Marco Castelluccio [:marco]]

Requesting tracking as this is a recent regression, with a clear regression range, in an user-visible feature.

Comment 2

[:Paolo Amadini]

If this is the offending code, the issue might manifest itself only Linux:

https://searchfox.org/mozilla-central/source/toolkit/mozapps/handling/content/dialog.js#167

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1498934: Fix handler app enumeration issue. r=Gijs

Since gioApps here is a plain nsIArray, it doesn't have any intrinsic type
information, and we only get the correct interfaces if they've already been
queried. Adding an interface parameter to the enumerate() call fixes the
issue.

This code is apparently untested, or incompletely tested.

Comment 4

[:Paolo Amadini]

Yes, we have little automated testing of this code.

Can the changes in bug 1484496 or bug 1484373 affect other uses of enumerate() without type information?

https://searchfox.org/mozilla-central/search?q=.enumerate()&case=false&regexp=false&path=*.js

Comment 5

[Justin Dolske [:Dolske]]

Assuming assignee based on patch.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10476448b17b53befefdf4eeb6d7278bd02401a9
Bug 1498934: Fix handler app enumeration issue. r=Gijs

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/10476448b17b

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

I'd like QA to verify this patch so as that we can evaluate it for uplift.

Kris, could you request uplift for beta and release channels? This seems like a good candidate for a dot-release ride-along. Thanks

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 9019750 [details]
Bug 1498934: Fix handler app enumeration issue. r=Gijs

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1484496

User impact if declined: This issue prevents some users from opening certain kinds of links in external apps.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0. Original reporter should try to verify, since it's not easily reproducible.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It's a trivial change to make sure we query the correct interface when iterating over nsIArray elements.

String changes made/needed: None.

Comment 10

[Marco Castelluccio [:marco]]

Verified fixed.

Comment 11

[Marco Castelluccio [:marco]]

Note, this probably also fixes bug 1498932, which is easily reproducible in a clean profile.

Comment 12

[Rares Doghi, Desktop QA]

Hi, I tried reproducing this issue but without any luck, the error I'm getting in Browser Console is "TypeError: docShell.failedChannel is null" and not "TypeError: handler.equals is not a function" and I was always able to open the magnet link in a new tab or a new window without any problems except that instead of the Undefined options I'm just getting the:
 
"The address wasn’t understood

Firefox doesn’t know how to open this address, because one of the following protocols (magnet) isn’t associated with any program or is not allowed in this context."

Based on Comment 10 I will mark this issue accordingly.

Comment 13

[:Paolo Amadini]

Kris, can the changes in bug 1484496 or bug 1484373 affect other uses of enumerate() without type information, or are we good here?

https://searchfox.org/mozilla-central/search?q=.enumerate()&case=false&regexp=false&path=*.js

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9019750 [details]
Bug 1498934: Fix handler app enumeration issue. r=Gijs

[Triage Comment]
Fixes issues with users being unable to open some links with external handler apps. Approved for 64.0b6.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c96f6ed751bf

Comment 16

[Rares Doghi, Desktop QA]

Hi Marco, can you please verify the beta version of Firefox as well for this issue, here is our latest beta: https://www.mozilla.org/en-US/firefox/channel/desktop/

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9019750 [details]
Bug 1498934: Fix handler app enumeration issue. r=Gijs

Fixes issues with users being unable to open some links with external handler apps. One liner, uplift verified on beta, approved for 63.0.3.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/bb7395925fab

Comment 19

[Rares Doghi, Desktop QA]

Hi Marco, Thanks for all the help, If it's not too much trouble can you please recheck this issue on the latest release version of Firefox? you have the build here: http://archive.mozilla.org/pub/firefox/candidates/63.0.3-candidates/  it will be much appreciated :)

Comment 20

[Marco Castelluccio [:marco]]

Totally forgot about this, luckily it appears to be working :)

The question to Kris still stands, perhaps it would be easier to email him directly or ping him on IRC.

Comment 21

[Kris Maglione [:kmag]]

(In reply to :Paolo Amadini from comment #13)
> Kris, can the changes in bug 1484496 or bug 1484373 affect other uses of
> enumerate() without type information, or are we good here?

It's possible, but unlikely. I updated the callers of the interfaces I added type information to individually. I just slightly misunderstood the way this interface worked, because it's weirdly-implemented.