Fix nsBrowserAccess.openURI()

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
General
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 53.0

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 months ago
It doesn't work when you try to open a URL:

Since 'referrer' has block scope, it is always undefined at line 693, so an exception is thrown and we end up in the 'catch' without opening anything:

https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mail/base/content/mailWindow.js#693
(Assignee)

Comment 1

7 months ago
Created attachment 8815353 [details] [diff] [review]
1321013-fix-openURI.patch (v1)

Here's the obvious fix. Stand-by for a debug patch to see it working ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8815353 - Flags: review?(acelists)
(Assignee)

Comment 2

7 months ago
Created attachment 8815364 [details] [diff] [review]
1321013-debug-patch.patch

This is misusing MailSetCharacterSet() as a test function.

Set the charset of a message to Unicode and BMO will open in a new tab. Set any other charset and Google will open.

This exercises two distinct cases internal cases.

Note that openURI() is also called when going to Get Add-ons, clicking on any add-on that's suggested, for example "Mail Merge", and then clicking onto a link, for example this one:
Website https://addons.mozilla.org/addon/mail-merge/

Comment 3

7 months ago
Comment on attachment 8815353 [details] [diff] [review]
1321013-fix-openURI.patch (v1)

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

Where do we get a proper JS compiler that catches this in advance? :)

Nice find. Looks like this was there even at the time of last change of that line in 2012.
Attachment #8815353 - Flags: review?(acelists) → review+
(Assignee)

Comment 4

7 months ago
C-C (TB 53): https://hg.mozilla.org/comm-central/rev/5bb2d288ae3ed011048a8a405eadad245c2448be
C-A (TB 52): https://hg.mozilla.org/releases/comm-aurora/rev/5edaddc025cbcf250be0ca07342d85879d9fa0ba
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-thunderbird52: --- → fixed
status-thunderbird53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Updated

7 months ago
Attachment #8815353 - Flags: approval-comm-aurora+
Any reason this was pushed DONTBUILD? I think we should be building normally even on minor changes, in order to catch potential m-c issues earlier. While it is nice to save some build time, I don't think we should be doing this on normal pushes.
(Assignee)

Comment 6

7 months ago
I have adopted this scheme:

If the push is minor and I'm sure it won't be covered by any test, I push with DONTBUILD unless between the last full build and my push there was an M-C merge.

Take a few cases:

This case:
Pushed Sat Dec 3, 20:10:33
Previous full build: Sat Dec 3, 18:56:47
Last M-C merge: Sat Dec 03 06:17:44
Decision: DONTBUILD

A previous case:
Pushed Sat Dec 3, 8:15:22
Previous full build: Sat Dec 3, 0:38:38
Last M-C merge: Sat Dec 03 06:17:44
Decision: BUILD despite trivial change (one JS line)

Believe me, I'm the first one who wants to see new bustage ;-)
But it doesn't make sense to build the exact same binaries twice and run the exact same tests.

There are other points to consider: These days we have a daily block list update on all repositories, so anything you push will be picked up within 24 hours. Before that wasn't the case, there the Dailies were built on the previous non-DONTBUILD pushes for days. So I'm more inclined to push to C-A and C-B with DONTBUILD than before.
You need to log in before you can comment on or make changes to this bug.