Closed Bug 342623 Opened 18 years ago Closed 16 years ago

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

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

[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
[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
[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
Attached patch (Av1) <imageContextOverlay.xul> (obsolete) — Splinter Review
Fix warning(s).
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #332466 - Flags: review?(iann_bugzilla)
Severity: trivial → minor
Flags: in-testsuite-
QA Contact: image-blocking
(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.
Attached patch (Av2) <imageContextOverlay.xul> (obsolete) — Splinter Review
[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 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+
Attached patch (Av3) <imageContextOverlay.xul> (obsolete) — Splinter Review
Av2, with comment 6 suggestion(s).
Attachment #332639 - Attachment is obsolete: true
Keywords: checkin-needed
(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 ?
Keywords: checkin-needed
It probably comes from permissionsNavigatorOverlay.xul, so if you remove the definition, imageContextOverlay.xul will probably fail in mailnews.
Attached patch (Av3a) <imageContextOverlay.xul> (obsolete) — Splinter Review
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)
(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.
Attachment #333048 - Flags: review?(iann_bugzilla) → review-
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)
Blocks: 450033
(In reply to comment #12)
> *feature removal in a new bug.

I filed bug 450033.
Attachment #333161 - Flags: review?(iann_bugzilla) → review+
Keywords: checkin-needed
Component: Image Blocking → Passwords & Permissions
Product: Core → SeaMonkey
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #333161 - Attachment description: (Av3b) <imageContextOverlay.xul> → (Av3b) <imageContextOverlay.xul> [Checkin: Comment 14]
Per comment 6 suggestion(s).
Attachment #333467 - Flags: review?(iann_bugzilla)
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+
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 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]
Target Milestone: mozilla1.9.1a2 → seamonkey2.0alpha
You need to log in before you can comment on or make changes to this bug.