In <imageContextOverlay.xul>, "Warning: assignment to undeclared variable uri" then "NS_ERROR_FAILURE"

RESOLVED FIXED in seamonkey2.0a1

Status

SeaMonkey
Passwords & Permissions
--
minor
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.0a1
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

12 years ago
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060623 SeaMonkey/1.1a] (nightly) (W98SE)

1. Load <about:>.

2. Context-click on the <about:logo> image,
and select 'Block Images from This Server'.
[
Warning: assignment to undeclared variable uri
Source File: chrome://communicator/content/permissions/imageContextOverlay.xul
Line: 83
]
and 'scheme:about / site cannot load images' is added to the Image Manager.

3. Context-click on the <about:logo> image again,
and select 'Unblock Images from This Server'.
[
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://communicator/content/permissions/imageContextOverlay.xul :: anonymous :: line 99"  data: no]
]
and the Image Manager setting is not removed.

Expected results:
2. This seems a trivial code fix, and not affecting the user behaviour.
3. The is more serious, and makes me wonder if the 'scheme:about' shouldn't be filtered in the first place ... as in "never blockable" !? (or maybe some people do want to "disable" this scheme ?)

Workaround:
3. Remove the entry via the Image Manager.
Assignee: security-bugs → nobody
(Assignee)

Comment 1

12 years ago
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.2pre) Gecko/20061229 SeaMonkey/1.1] (nightly) (W2Ksp4)

Bug still there.

(OS: Windows 98 -> Windows 2000, as I'm upgrading my computer.)
OS: Windows 98 → Windows 2000
(Assignee)

Comment 2

10 years ago
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070515 SeaMonkey/1.5a] (nightly) (W2Ksp4)

Full bug was still there.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008072102 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/2008080519 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

This option, with these steps, is not available from the context menu anymore :-)
I assume bug 93390 fixed step 3 issue.

***

Step 2 warning is still there, with an image from a server:
{{
Warning: assignment to undeclared variable uri
Source File: chrome://communicator/content/permissions/imageContextOverlay.xul
Line: 64
}}
Severity: minor → trivial
Depends on: 93390
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1a2
Version: 1.8 Branch → Trunk
(Assignee)

Comment 3

10 years ago
Created attachment 332466 [details] [diff] [review]
(Av1) <imageContextOverlay.xul>

Fix warning(s).
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #332466 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

10 years ago
Severity: trivial → minor
Flags: in-testsuite-
QA Contact: image-blocking

Comment 4

10 years ago
(In reply to comment #3)
> Created an attachment (id=332466) [details]
> (Av1) <imageContextOverlay.xul>
> 
> Fix warning(s).
> 
Whilst you are here you could make how the two functions get permissionmanager the same - interesting that Fx have combined the two functions and determine the add/remove by calling a single function with an argument - http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/nsContextMenu.js#972 - but that is outside the scope of this bug I suspect.
(Assignee)

Comment 5

10 years ago
Created attachment 332639 [details] [diff] [review]
(Av2) <imageContextOverlay.xul>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/2008080202 SeaMonkey/2.0a1pre] (home, debug default) (W2Ksp4)

Av1, with comment 4 suggestion(s).
Attachment #332466 - Attachment is obsolete: true
Attachment #332639 - Flags: review?(iann_bugzilla)
Attachment #332466 - Flags: review?(iann_bugzilla)

Comment 6

10 years ago
Comment on attachment 332639 [details] [diff] [review]
(Av2) <imageContextOverlay.xul>

># HG changeset patch
># User Serge Gautherie <sgautherie.bz@free.fr>
># Date 1218071674 -7200
># Node ID a83eb7babe14701c32f9f7ff4d6b88ebd6235fff
># Parent  b7ca9a3a097432ae6235bf86fc22085e2aec1194
># Parent  b77c39fd4acae24c8e1f99aed125bcf8915984cb
>--- a/suite/common/permissions/imageContextOverlay.xul
>+++ b/suite/common/permissions/imageContextOverlay.xul
>-        const nsIPermissionManager = Components.interfaces.nsIPermissionManager;
>+      // Block/Unblock image from loading in the future.
>+      toggleImageBlocking: function CCM_toggleImageBlocking(aBlock) {
>         var permissionmanager =
>           Components.classes["@mozilla.org/permissionmanager;1"]
>+                    .getService(Components.interfaces.nsIPermissionManager);
I'd be happier keeping a const nsIPermissionManager in the function as I think it will look tidier. 
>         if (!permissionmanager) {
>           return;
>         }
> 
>+        const uri = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService)
>+                              .newURI(gContextMenu.imageURL, null, null);
>+        if (aBlock)
>+          permissionmanager.add(
>+            uri, "image",
>+            Components.interfaces.nsIPermissionManager.DENY_ACTION);
With const nsIPermissionManager the uri, "image", can go on the line above and DENY_ACTION below.
>@@ -139,26 +128,26 @@
>     window.addEventListener("load", cookieContextMenu.addContextMenuItemListeners, false);
> 
>    ]]>
>   </script>         
Nit: White space after the </script>

r=me with those changes
Attachment #332639 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 7

10 years ago
Created attachment 332848 [details] [diff] [review]
(Av3) <imageContextOverlay.xul>

Av2, with comment 6 suggestion(s).
Attachment #332639 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Comment 8

10 years ago
(In reply to comment #6)
> I'd be happier keeping a const nsIPermissionManager in the function as I think
> it will look tidier. 

Actually, while preparing a white-space cleanup patch, I noticed
"if (permissionmanager.testPermission(uri, "image") == nsIPermissionManager.DENY_ACTION)"
and I verified that a |const nsIPermissionManager| is already globally available to this file.
(But I did not search where it comes from.)
Do you prefer to use it, or to fix that other line ?
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 9

10 years ago
It probably comes from permissionsNavigatorOverlay.xul, so if you remove the definition, imageContextOverlay.xul will probably fail in mailnews.
(Assignee)

Comment 10

10 years ago
Created attachment 333048 [details] [diff] [review]
(Av3a) <imageContextOverlay.xul>

Av3, with comment 9 suggestion(s).

*I don't know how to test this feature in MailNews.
*I added a rewrite, noticed while preparing the white space cleanup patch.
Attachment #332848 - Attachment is obsolete: true
Attachment #333048 - Flags: review?(iann_bugzilla)

Comment 11

10 years ago
(In reply to comment #10)
> Created an attachment (id=333048) [details]
> (Av3a) <imageContextOverlay.xul>
> 
> Av3, with comment 9 suggestion(s).
> 
> *I don't know how to test this feature in MailNews.
> *I added a rewrite, noticed while preparing the white space cleanup patch.
> 
Ah looking closer - http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/mailContextMenus.js#521
Even though we have context items for mailnews we hide them because we never use them. How we block images in mailnews has moved on so in theory we could remove these context menuitems (most of http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/suite/common/permissions&command=DIFF_FRAMESET&file=imageContextOverlay.xul&rev2=1.16&rev1=1.15 but leave the |if (contextPopup)| check) and the changes to mailContextMenus.js (http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/mailnews/base/resources/content&command=DIFF_FRAMESET&file=mailContextMenus.js&rev2=1.35&rev1=1.34)
but probably best you get an sr= on those changes.
Does mean that you can remove the |const nsIPermissionManager| from the function though.

I think the added rewrite in the latest version of the patch is unnecessary, it makes what is happening in the code less clear.

Updated

10 years ago
Attachment #333048 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 12

10 years ago
Created attachment 333161 [details] [diff] [review]
(Av3b) <imageContextOverlay.xul>
[Checkin: Comment 14]

Av3a, with comment 11 suggestion(s).

Let's separate the issues:
*code merge then white space removals here.
*feature removal in a new bug.
Attachment #333048 - Attachment is obsolete: true
Attachment #333161 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

10 years ago
Blocks: 450033
(Assignee)

Comment 13

10 years ago
(In reply to comment #12)
> *feature removal in a new bug.

I filed bug 450033.

Updated

10 years ago
Attachment #333161 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Updated

10 years ago
Component: Image Blocking → Passwords & Permissions
Product: Core → SeaMonkey

Updated

10 years ago
QA Contact: image-blocking → general
pushing to ssh://hg.mozilla.org/comm-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #333161 - Attachment description: (Av3b) <imageContextOverlay.xul> → (Av3b) <imageContextOverlay.xul> [Checkin: Comment 14]
(Assignee)

Comment 15

10 years ago
Created attachment 333467 [details] [diff] [review]
(Bv1) <imageContextOverlay.xul> white space cleanup

Per comment 6 suggestion(s).
Attachment #333467 - Flags: review?(iann_bugzilla)

Comment 16

10 years ago
Comment on attachment 333467 [details] [diff] [review]
(Bv1) <imageContextOverlay.xul> white space cleanup

>-                var serverLabelEllipsis = pref.getComplexValue("intl.ellipsis",
>-                                     Components.interfaces.nsIPrefLocalizedString).data;               
>+                var serverLabelEllipsis =
>+                  pref.getComplexValue(
>+                        "intl.ellipsis",
>+                        Components.interfaces.nsIPrefLocalizedString)
>+                      .data;
The only change I'm not happy with is this one, it looks more ugly than what was there before, maybe just have the Components line indented but not as far as the original?

r=me with that sorted.
Attachment #333467 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 17

10 years ago
Created attachment 333477 [details] [diff] [review]
(Bv1a) <imageContextOverlay.xul> white space cleanup [Checkin: Comment 18]

Bv1, with comment 16 suggestion(s).

As I don't like it "moved only", I'll eventually leave it where it is ;->
Attachment #333467 - Attachment is obsolete: true

Comment 18

10 years ago
Comment on attachment 333477 [details] [diff] [review]
(Bv1a) <imageContextOverlay.xul> white space cleanup [Checkin: Comment 18]

http://hg.mozilla.org/comm-central/index.cgi/rev/701d856ed028
Attachment #333477 - Attachment description: (Bv1a) <imageContextOverlay.xul> white space cleanup → (Bv1a) <imageContextOverlay.xul> white space cleanup [Checkin: Comment 18]

Updated

10 years ago
Target Milestone: mozilla1.9.1a2 → seamonkey2.0alpha
You need to log in before you can comment on or make changes to this bug.