1223741 - Updating to 2.39B allow images not working

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Steve Manser]

Attachment: 151111 SeaMonkey Mail issue.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/601.2.7 (KHTML, like Gecko) Version/9.0.1 Safari/601.2.7

Steps to reproduce:

Upgraded from 2.38 to 2.39 (UK British localisation), go into mail, view mail with images, see yellow bar showing blocked content


Actual results:

Choose to show content from email address, has no effect so have to select 'Show remote content for this message' to see it, however you have to do this for every message as it is not retained choice


Expected results:

When I selected show content from this email address it should have done this and then allowed content from that address for any future emails from this source (worked beautifully in SeaMonkey 2.38.0 (UK British))

Comment 1

[Rainer Bielefeld]

Symptoms sound similar to "Bug 1135387 - Checkmark "Allow remote content" without effect", but that problem appeared long before 2.39

@reporter:
please test relation to Bug 1135387 and contribute a step by step instruction containing every key press and every mouse click how to reproduce your problem due to <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines>  (similar to report in Bug 1139273)

Comment 2

[Rainer Bielefeld]

I read several comments that 2.38 was the only version where  "Allow remote content" worked correctly. 

I wonder whether in 2.39 the old problem reappeared or a new one appeared (with old symptoms)

Comment 3

[Peter Schmidt]

Seem for me that the problem has to do with the format of "Permission Data Record".
I can see several scenarios:
If I do an "Allow remote content for ..." no "Permission Data Record" can be found neither under 2.39 as under 2.38 .
If I do an "Allow remote content for ..." under 2.38 no "Permission Data Record" can be found under 2.39 but can be seen under 2.38 .
I can see under 2.39 some "Permission Data Records" with "Load Images" entries that are not honored.
I have at least one "Permission Data Record" with "Load Images" entries that works under 2.39
(Sorry but I do not know under with version this entries where added.)

Comment 4

[Chuck Pfeiffer]

Worked fine until v 2.39, and for years at that.  Creating Remote Content group in Address Book years ago, then 2.39 upgrade and it does not hold.  Have to recheck Allow Remote Content every mail message.  
Very frustrating.

Comment 5

[Frank-Rainer Grahl (:frg)]

Could someone check if he/she sees a similar error in the web console when trying to unblock the content:

>> Timestamp: 3/30/2016 7:24:44 PM
>> Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) 
>> [nsIPermissionManager.add]
>> Source File: chrome://messenger/content/mailWindowOverlay.js
>> Line: 2794

Permission api did change in 2.39 and it looks like the code here is wrong. If I have some time I will look into it this weekend. 

Confirming bug. Tested on 2.45a1 x64 Windows 7.

Comment 6

[Frank-Rainer Grahl (:frg)]

Looked at it a bit more. I think this fails with https:// and other non http hosts currently. Thunderbird code is completely different here. Seamonkey needs either a big fix or a straight port.

Comment 7

[gtoole]

Confirmed in 2.39 W32 (XP sp3).  Also confirmed problem not present in 2.38.  Unfortunately this bug makes SeaMonkey Mail too frustrating for me to use everyday.  Have reverted to 2.38 and disabled auto updates until this bug is fixed.
Thanks for looking into it, guys.  Keep up the good work.  (Wish I could help, but I've been out of the software development biz for too many years.)

Comment 8

[Frank-Rainer Grahl (:frg)]

TB fixed this in bug 1193200. They added a workaround using "chrome://messenger/content/?email=" to allow email addresses to be whitelisted using the permissions api.

Ian, do we want to also do this or should we just make the website image permissions stick by replacing the internal use of hosts with origins and remove the option to whitelist email addresses? The later needs to be done anyway.

The fixed Data Manager will currently not like chrome:// urls but I plan to fix this and some quirks later anyway.

Comment 10

[gtoole]

See also Bug 995737 - Adapt seamonkey for the addressbook remote content policy change; use the permission manager instead of address book property - which was fixed in SM 2.38.  Remote content option is remembered in 2.38 but not in 2.39.  Regression?

Comment 11

[Frank-Rainer Grahl (:frg)]

I am quite sure the issues fixed in bug 995737 and popping up in 2.39 again will go away if the issue here is fixed. Will test when a patch is ready.

Comment 12

[Frank-Rainer Grahl (:frg)]

Attachment: 1223741-mailnews-part.patch

Port for the mail part of Bug 1193200. I do not use Seamonkey for mail. Tried it with my gmx account and imap. Unblocking seems to work and stick between sessions.

I added a debug preference. Let me know if this is ok and if yes can stay in. Helped me to see what content needs to be unblocked and what goes on. 

The fixed Data Manager in 2.45a1 just ignores the chrome:// permissions right now. I will think about it but will pursue this in a separate bug.

Comment 13

[Frank-Rainer Grahl (:frg)]

Attachment: 1223741-mailnews-part-V2.patch

Version 2 patch with logging reworked.

The Data Manager in 2.45 will not choke on the xul permission but as stated will be patched up to display it and other non httpx permissions properly in a followup bug.

Comment 14

[Ian Neal]

Comment on attachment 8742123 [details] [diff] [review]
1223741-mailnews-part-V2.patch

>     let menuitem = document.createElement("menuitem");
>     menuitem.setAttribute("label",
>+    messengerBundle.getFormattedString("remoteContentAllow",
>+      [origin.replace("chrome://messenger/content/?email=", "")]));
Nit: indentation

> function allowRemoteContentForURI(aItem)
> {
>-  var host = aItem.getAttribute("host");
>-  if (!host)
>+  log.debug("Start allowRemoteContentForURI");
>+
>+  var origin = aItem.getAttribute("value");
>+
>+  if (!origin)
>     return;
> 
>-  var scheme = host.includes("@") ? "mailto:" : "http://";
>-  var uri = Services.io.newURI(scheme + host, null, null);
Would there still be items that need http:// ?

Comment 15

[Frank-Rainer Grahl (:frg)]

Attachment: 1223741-mailnews-part-V2a.patch

Indention fix only.

>> Would there still be items that need http:// ?

Imho no. Previously the set did consist of only hosts. With the fix we switch it to origins made out of URIs. If some weirdo URI is passed to setRemoteContentMsg and a host couldn't be found uri.spec is used. The permissions api seems to eat almost everything so even files and chrome works as long as it has a ://. If it does fail here the content will not be unblocked but if someone finds such a case we might meed another workaround like with mailto anyway and not just a http://. For any other case we need to use the original scheme which must not be replaced by something else or unblocking will not work.

Comment 16

[Philip Chee]

Comment on attachment 8744717 [details] [diff] [review]
1223741-mailnews-part-V2a.patch

> +    let origin;
use var here.

> +    try
> +    {
> +      origin = aContentURI.scheme + "://" + aContentURI.hostPort;
> +    }
> +    // No hostport so likely a special url. Try to use the whole url and see
> +    // what comes of it.
> +    catch (e)
> +    {
> +      origin = aContentURI.spec;
> +    }

This could be written as:
var origin = aContentURI.spec;
try
{
  origin = aContentURI.scheme + "://" + aContentURI.hostPort;
}
catch (e) { };

> +  let authorEmailAddressURI = Services.io.newURI(
> +    "chrome://messenger/content/?email=" + authorEmailAddress, null, null);
Use "var" here. Suggest use shorter variable names e.g. emailURL, Principal.
var emailURI = Services.io.newURI(
  "chrome://messenger/content/?email=" + authorEmailAddress, null, null);

> +  let mailPrincipal = Services.scriptSecurityManager
> +                              .createCodebasePrincipal(authorEmailAddressURI, {});
var principal = Services.scriptSecurityManager
                        .createCodebasePrincipal(emailURI, {});

> -    hosts.unshift(authorEmailAddress);
> +  origins.splice(0, 0, mailPrincipal.origin);
Please use unshift here.

>      let menuitem = document.createElement("menuitem");
>      menuitem.setAttribute("label",
> -      messengerBundle.getFormattedString("remoteContentAllow", [host]));
> -    menuitem.setAttribute("host", host);

> +    messengerBundle.getFormattedString("remoteContentAllow",
> +      [origin.replace("chrome://messenger/content/?email=", "")]));
I think you need to indent the above two lines two more spaces.

> +    menuitem.setAttribute("value", origin);

I'd like to see a new patch with all the debug statements removed before checkin. Thanks.

Comment 17

[Frank-Rainer Grahl (:frg)]

Attachment: 1223741-mailnews-part-V3.patch

>> This could be written as:
>> var origin = aContentURI.spec; 
>> ...

Changed it but was intentional originally. I am always uneasy with these constructs because I am not sure if there aren't rare cases where the original variable is somehow touched or altered during the assignment when it fails. 

>> I'd like to see a new patch with all the debug statements removed before checkin.

I am a fan of them unless they are in performance critical code which wasn't the case here but removed. You are the boss :)

If the patch is ok it should go into c-a too. It itself would be safe for the other branches too but I didn't test it with the broken Data Manager in them yet.

Comment 18

[Philip Chee]

Comment on attachment 8745485 [details] [diff] [review]
1223741-mailnews-part-V3.patch

> +  let emailURI = Services.io.newURI(
s/let/var/
> +    "chrome://messenger/content/?email=" + authorEmailAddress, null, null);
> +  let principal = Services.scriptSecurityManager
s/let/var/

>      menuitem.setAttribute("label",
> -      messengerBundle.getFormattedString("remoteContentAllow", [host]));
> -    menuitem.setAttribute("host", host);
> +      messengerBundle.getFormattedString("remoteContentAllow",
> +        [origin.replace("chrome://messenger/content/?email=", "")]));
I've decided that this lacks a certain clarity. Lets use something like this:

let host = origin.replace("chrome://messenger/content/?email=", "");
let hostString = messengerBundle.getFormattedString("remoteContentAllow", [host]);
menuitem.setAttribute("label", hostString);

r=me with the above nits fixed.

Comment 19

[Frank-Rainer Grahl (:frg)]

Attachment: 1223741-mailnews-part-V3a.patch

nits fixed.

review+ from Philip Chee carried over.

Comment 20

[Frank-Rainer Grahl (:frg)]

checked-in

https://hg.mozilla.org/comm-central/rev/b120a5b361b8

Comment 21

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8745485 [details] [diff] [review]
1223741-mailnews-part-V3.patch

[Approval Request Comment]
Regression caused by (bug #): 1165263
User impact if declined: remote content can not be unblocked.
Testing completed (on m-c, etc.): c-a
Risk to taking this patch (and alternatives if risky): none its borken already
String changes made by this patch: none

Comment 22

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8747458 [details] [diff] [review]
1223741-mailnews-part-V3a.patch

[Approval Request Comment]
Regression caused by (bug #): Bug 1165263
User impact if declined: remote content can not be unblocked.
Testing completed (on m-c, etc.): c-a
Risk to taking this patch (and alternatives if risky): none its broken already
String changes made by this patch: none

Comment 23

[Frank-Rainer Grahl (:frg)]

Checked into c-a:

https://hg.mozilla.org/releases/comm-aurora/rev/b9e78456cca7

I am leaving the bug open to see if it can be pushed to beta and release too. Need to check that the unfixed data manager does not choke on it.

Comment 24

[Frank-Rainer Grahl (:frg)]

Attachment: Capture.PNG

[Approval Request Comment]
Regression caused by (bug #): Bug 1165263
User impact if declined: remote content can not be unblocked.
Testing completed (on m-c, etc.): c-r c-b
Risk to taking this patch (and alternatives if risky): none its broken already
String changes made by this patch: none

The unfixed Data Manager displays a messenger host for the permission and lists it under a messenger domain. The permission can not be removed but I do not consider this a big problem because permission handling is practically broken for any non http scheme in 2.43 and 2.44 anyway :)

Tested with 2.43 VS2015 x64 build under Windows 10.

Comment 25

[Philip Chee]

Comment on attachment 8747465 [details]
Capture.PNG

a=me CLOSED TREE in case you need it.

Comment 26

[Frank-Rainer Grahl (:frg)]

Done
https://hg.mozilla.org/releases/comm-beta/rev/fc1290dc036d
https://hg.mozilla.org/releases/comm-release/rev/3dec13956227
https://hg.mozilla.org/releases/comm-esr45/rev/e9db074af6c9