Closed
Bug 1223741
Opened 9 years ago
Closed 9 years ago
Updating to 2.39B allow images not working
Categories
(SeaMonkey :: MailNews: Message Display, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.43
People
(Reporter: smanser, Assigned: frg)
References
Details
Attachments
(3 files, 4 obsolete files)
129.68 KB,
image/png
|
Details | |
5.54 KB,
patch
|
frg
:
review+
ewong
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
19.06 KB,
image/png
|
philip.chee
:
approval-comm-beta+
philip.chee
:
approval-comm-release+
|
Details |
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•9 years ago
|
||
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)
Flags: needinfo?(smanser)
See Also: → 1135387
Comment 2•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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.
![]() |
Assignee | |
Comment 5•9 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
![]() |
Assignee | |
Comment 6•9 years ago
|
||
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.
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.)
![]() |
Assignee | |
Comment 8•9 years ago
|
||
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.
Flags: needinfo?(iann_bugzilla)
See Also: → 1193200
![]() |
Assignee | |
Updated•9 years ago
|
Assignee: nobody → frgrahl
Comment 10•9 years ago
|
||
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?
![]() |
Assignee | |
Comment 11•9 years ago
|
||
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.
See Also: → 995737
![]() |
Assignee | |
Comment 12•9 years ago
|
||
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.
Attachment #8739685 -
Flags: review?(mnyromyr)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8739685 -
Flags: review?(mnyromyr) → review?(philip.chee)
![]() |
Assignee | |
Comment 13•9 years ago
|
||
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.
Attachment #8739685 -
Attachment is obsolete: true
Attachment #8739685 -
Flags: review?(philip.chee)
Attachment #8742123 -
Flags: review?(philip.chee)
Comment 14•9 years ago
|
||
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:// ?
Flags: needinfo?(iann_bugzilla) → needinfo?(frgrahl)
![]() |
Assignee | |
Comment 15•9 years ago
|
||
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.
Attachment #8742123 -
Attachment is obsolete: true
Attachment #8742123 -
Flags: review?(philip.chee)
Flags: needinfo?(frgrahl)
Attachment #8744717 -
Flags: review?(philip.chee)
![]() |
||
Comment 16•9 years ago
|
||
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.
Attachment #8744717 -
Flags: review?(philip.chee) → review-
![]() |
Assignee | |
Comment 17•9 years ago
|
||
>> 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.
Attachment #8744717 -
Attachment is obsolete: true
Attachment #8745485 -
Flags: review?(philip.chee)
![]() |
||
Comment 18•9 years ago
|
||
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.
Attachment #8745485 -
Flags: review?(philip.chee) → review+
![]() |
Assignee | |
Comment 19•9 years ago
|
||
nits fixed.
review+ from Philip Chee carried over.
Attachment #8745485 -
Attachment is obsolete: true
Flags: needinfo?(smanser)
Attachment #8747458 -
Flags: review+
![]() |
Assignee | |
Comment 20•9 years ago
|
||
Keywords: leave-open
![]() |
Assignee | |
Comment 21•9 years ago
|
||
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
Attachment #8745485 -
Flags: approval-comm-aurora?
![]() |
Assignee | |
Comment 22•9 years ago
|
||
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
Attachment #8747458 -
Flags: approval-comm-aurora?
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8745485 -
Flags: approval-comm-aurora?
![]() |
||
Updated•9 years ago
|
Attachment #8747458 -
Flags: approval-comm-aurora? → approval-comm-aurora+
![]() |
Assignee | |
Comment 23•9 years ago
|
||
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.
![]() |
Assignee | |
Comment 24•9 years ago
|
||
[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.
Attachment #8747465 -
Flags: approval-comm-release?
Attachment #8747465 -
Flags: approval-comm-beta?
![]() |
||
Comment 25•9 years ago
|
||
Comment on attachment 8747465 [details]
Capture.PNG
a=me CLOSED TREE in case you need it.
Attachment #8747465 -
Flags: approval-comm-release?
Attachment #8747465 -
Flags: approval-comm-release+
Attachment #8747465 -
Flags: approval-comm-beta?
Attachment #8747465 -
Flags: approval-comm-beta+
![]() |
Assignee | |
Comment 26•9 years ago
|
||
![]() |
Assignee | |
Updated•8 years ago
|
Target Milestone: --- → seamonkey2.43
You need to log in
before you can comment on or make changes to this bug.
Description
•