1540663 - Clicking on links in email messages doesn't work in nightly

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Kamens]

As of 2019-03-31 nightly build, when I click on a link in an email message, nothing happens, and this error gets logged in the error console:

NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI] contentAreaClick.js:182
openLinkExternally chrome://communicator/content/contentAreaClick.js:182
contentAreaClick chrome://communicator/content/contentAreaClick.js:165
onclick chrome://messenger/content/messenger.xul:1

Comment 1

[Magnus Melin [:mkmelin]]

Please remember to mark regressions as such.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1540663_click_fail.patch

I'll add more tests for this in another bug. Would be good to get this into nightly.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9055042 [details] [diff] [review]
bug1540663_click_fail.patch

Review of attachment 9055042 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/phishingDetector.js
@@ +35,5 @@
>      this.mDisallowFormActions = Services.prefs.getBoolPref("mail.phishing.detection.disallow_form_actions");
>    },
>  
>    /**
> +   * Analyzethe currently loaded message in the message pane, looking for signs

Missing space.

@@ +46,3 @@
>     *
> +   * @return asynchronously calls gMessageNotificationBar.setPhishingMsg if the
> +   *          message is identified as a scam.

Indent one to the left.

@@ +197,5 @@
>     */
>    warnOnSuspiciousLinkClick(aUrl, aLinkText) {
> +    if (!this.analyzeUrl(aUrl, aLinkText)) {
> +      return 2; // No problem with the url. Allow it to load.
> +    }

I can't say that I understand the reshuffle, but the function now never returns 0 any more.

Comment 4

[Jorg K (CEST = GMT+2)]

As per IRC: 10:49:10 - mkmelin: no, it does return 0, if you end up with a confirmEx and the user choses 0

Comment 5

[Magnus Melin [:mkmelin]]

The most important part of the reshuffle is that we now analyse first. Then we must make sure the aLinkText is actually an url (since the method is called for all urls), or we try to make arbitrary text a url and that fails of course.

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: bug1540663_click_fail.patch

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9055050 [details] [diff] [review]
bug1540663_click_fail.patch

Review of attachment 9055050 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/phishingDetector.js
@@ +202,3 @@
>      let bundle = document.getElementById("bundle_messenger");
>  
> +    // Analysis said there's a problem. Link mismatch then?

"Link mismatch then?" is confusing. The comment suggests that you're going to test for a mismatch, but analyzeUrl() already did. I suggest to remove that part of the comment.

@@ +202,4 @@
>      let bundle = document.getElementById("bundle_messenger");
>  
> +    // Analysis said there's a problem. Link mismatch then?
> +    if (aLinkText && /^https?:/.test(aLinkText)) {

There is an i for case-insensitive match missing here. Other parts of the code use xxx.schemeIs("http") || xxx.schemeIs("https")

Comment 8

[Magnus Melin [:mkmelin]]

schemeIs can only be used for URIs, not strings.
I've updated the comment.

I think some more refactoring would make sense in this file, but... for another bug.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5db9b0e6efd1
Restore clicking on links in email messages to working order after bug 1476428. r=jorgk

Comment 10

[Jorg K (CEST = GMT+2)]

Due to the time constraints, I landed it with the changes I had suggested, so with the "i" and the comment removed. Next time, please make yourself available on IRC.