415218 - Fix uses of IO.newURI to use nsIIOService instead

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Reed Loden [:reed]]

Attachment: wip

Bug 414901 removed the scriptable IO, which means everywhere IO.newURI was being used now doesn't work. All those uses need to be changed to use nsIIOService instead. This affects context menus, places, content handling, and maybe other things.

Here's a starter patch that philor and I put together, but it's not complete, and we are both going to bed. Somebody please pick this up, complete the patch, get it reviewed and approved, and land it. Thanks!

Comment 1

[Hussam Al-Tayeb]

It seems Bug 415193 is another side effect because it is fixed with the backout. Do I mark as duplicate

Comment 2

[Hussam Al-Tayeb]

On second thought, I might want to test first. Sorry for the spam.

Comment 3

[(no longer active)]

Taking over.

Comment 4

[Neil Deakin]

Comment on attachment 300839 [details] [diff] [review]
wip

Looks OK, there are places though where you use Ci/Cc where they may not be defined. Unless the file uses them elsewhere, we should use Components.classes/interfaces.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

This patch is modified a bit to fix a few problems with the patch provided by Reed.  Thanks for the original work, Reed!

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Ehsan - will you have time to get to the rest of the patch today? If not, Mano, do you think you could take it the rest of the way?

Comment 7

[Asaf Romano (gone)]

Comment on attachment 300879 [details] [diff] [review]
Patch (v1)

>Index: browser/base/content/browser-places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-places.js,v
>retrieving revision 1.91
>diff -u -8 -p -r1.91 browser-places.js
>--- browser/base/content/browser-places.js	31 Jan 2008 21:40:37 -0000	1.91
>+++ browser/base/content/browser-places.js	1 Feb 2008 17:03:55 -0000
>@@ -417,17 +417,19 @@ var PlacesCommandHook = {
>    *        The folder in which to create a new bookmark if aURL isn't
>    *        bookmarked.
>    * @param aURL (string)
>    *        the address of the link target
>    * @param aTitle
>    *        The link text
>    */
>   bookmarkLink: function PCH_bookmarkLink(aParent, aURL, aTitle) {
>-    var linkURI = IO.newURI(aURL)
>+    var linkURI = Cc["@mozilla.org/network/io-service;1"].
>+                  getService(Ci.nsIIOService).
>+                  newURI(aURL, null, null);


please use makeURI here.
> 
>Index: browser/base/content/utilityOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/utilityOverlay.js,v
>retrieving revision 1.59
>diff -u -8 -p -r1.59 utilityOverlay.js
>--- browser/base/content/utilityOverlay.js	9 Jan 2008 07:37:06 -0000	1.59
>+++ browser/base/content/utilityOverlay.js	1 Feb 2008 17:03:56 -0000
>@@ -492,18 +492,20 @@ function isElementVisible(aElement)
>   // If aElement or a direct or indirect parent is hidden or collapsed,
>   // height, width or both will be 0.
>   var bo = aElement.boxObject;
>   return (bo.height > 0 && bo.width > 0);
> }
> 
> function makeURLAbsolute(aBase, aUrl)
> {
>-  // Note:  IO.newURI(aUri) will throw if aUri is not a valid URI
>-  return IO.newURI(aUrl, null, IO.newURI(aBase)).spec;
>+  // Note:  ios.newURI() will throw if aUri is not a valid URI
>+  var ios = Components.classes["@mozilla.org/network/io-service;1"].
>+            getService(Components.interfaces.nsIIOService);
>+  return ios.newURI(aUrl, null, ios.newURI(aBase, null, null)).spec;

here too.

>Index: browser/components/places/content/editBookmarkOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.js,v
>retrieving revision 1.20
>diff -u -8 -p -r1.20 editBookmarkOverlay.js
>--- browser/components/places/content/editBookmarkOverlay.js	31 Jan 2008 19:58:03 -0000	1.20
>+++ browser/components/places/content/editBookmarkOverlay.js	1 Feb 2008 17:03:56 -0000
>@@ -799,17 +799,19 @@ var gEditItemOverlay = {
>           // clear undo stack
>           namePicker.editor.transactionManager.clear();
>         }
>       }
>       break;
>     case "uri":
>       var locationField = this._element("locationField");
>       if (locationField.value != aValue) {
>-        this._uri = IO.newURI(aValue);
>+        this._uri = Components.classes["@mozilla.org/network/io-service;1"].
>+                    getService(Components.interfaces.nsIIOService).
>+                    newURI(aValue, null, null);

use Cc/Ci

(you cannot use makeURI here becuase not all places windows include contentAreaUtils)

>Index: browser/components/places/content/utils.js
>===================================================================
>@@ -725,18 +727,18 @@ var PlacesUtils = {
>             var title = node.folder.title;
>             var annos = node.folder.annos;
>             var folderItemsTransactions =
>               getChildItemsTransactions(node.children);
>             txn = self.ptm.createFolder(title, -1, index, annos,
>                                         folderItemsTransactions);
>           }
>           else { // node is a livemark
>-            var feedURI = IO.newURI(node.uri.feed);
>-            var siteURI = IO.newURI(node.uri.site);
>+            var feedURI = this._uri(node.uri.feed);
>+            var siteURI = this._uri(node.uri.site);

self, not this.

>@@ -1857,56 +1859,56 @@ var PlacesUtils = {
>       runBatched: function(aUserData) {
>         delete self.leftPaneQueries;
>         self.leftPaneQueries = { };
> 
>         // Left Pane Root Folder
>         leftPaneRoot = self.bookmarks.createFolder(self.placesRootId, "", -1);
> 
>         // History Query
>-        let uri = IO.newURI("place:sort=4&");
>+        let uri = this._uri("place:sort=4&");
>         let title = self.getString("OrganizerQueryHistory");
>         let itemId = self.bookmarks.insertBookmark(leftPaneRoot, uri, -1, title);
>         self.annotations.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO,
>                                            "History", 0, EXPIRE_NEVER);
>         self.leftPaneQueries["History"] = itemId;
> 
>         // XXX: Downloads
> 
>         // Tags Query
>-        uri = IO.newURI("place:folder=" + self.tagsFolderId);
>+        uri = this._uri("place:folder=" + self.tagsFolderId);

ditto (there might be few more places).

Comment 8

[(no longer active)]

(In reply to comment #6)
> Ehsan - will you have time to get to the rest of the patch today? If not, Mano,
> do you think you could take it the rest of the way?

Actually it's tonight here!  :-)

Yes, but what do you mean by the rest of the patch?  I think I fixed all occurrences (<http://mxr.mozilla.org/firefox/search?string=IO.newURI&find=\.js$>)...

I'm currently working on a new patch based on comment 7...

Comment 9

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #7)
> please use makeURI here.

Done.

> use Cc/Ci

Done.

> (you cannot use makeURI here becuase not all places windows include
> contentAreaUtils)

Good tip (I was about to move ahead and change everything to makeURI) :-)

> self, not this.

Oops.  Fixed.

> ditto (there might be few more places).

I think I caught them all...

Comment 10

[(no longer active)]

Beltzner: please see comment 8.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 300884 [details] [diff] [review]
Patch (v2)

>Index: browser/base/content/browser-places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-places.js,v
>retrieving revision 1.91
>diff -u -8 -p -r1.91 browser-places.js
>--- browser/base/content/browser-places.js	31 Jan 2008 21:40:37 -0000	1.91
>+++ browser/base/content/browser-places.js	1 Feb 2008 17:16:48 -0000
>@@ -417,17 +417,17 @@ var PlacesCommandHook = {
>    *        The folder in which to create a new bookmark if aURL isn't
>    *        bookmarked.
>    * @param aURL (string)
>    *        the address of the link target
>    * @param aTitle
>    *        The link text
>    */
>   bookmarkLink: function PCH_bookmarkLink(aParent, aURL, aTitle) {
>-    var linkURI = IO.newURI(aURL)
>+    var linkURI = makeURI(aURL, null, null);

The last two arguments are optional.

>Index: browser/base/content/utilityOverlay.js
>===================================================================

> function makeURLAbsolute(aBase, aUrl)
> {
>-  // Note:  IO.newURI(aUri) will throw if aUri is not a valid URI
>-  return IO.newURI(aUrl, null, IO.newURI(aBase)).spec;
>+  // Note:  makeURI() will throw if aUri is not a valid URI
>+  return makeURI(aUrl, null, makeURI(aBase, null, null)).spec;
> }

ditto.

r=mano otherwise.

Comment 12

[(no longer active)]

Attachment: Patch (v2.1)

Addressing issues in comment 11 (carrying over mano's r+).

This needs approval for beta 3, because it blocks bug 414901 which needs to land for beta 3 as well.  This is critical to alert people that the scriptable IO APIs won't be present in 1.9 (see bug 414901 comment 0).

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 300899 [details] [diff] [review]
Patch (v2.1)

a=beltzner

Comment 14

[Phil Ringnalda (:philor)]

What with bug 414901 having broken big swaths of the app without turning anything orange (and in fact context menu and page info thing dealing with relative URIs had been broken for a month without orange or notice), this seems like a rather good candidate for some accompanying tests.

Comment 15

[Reed Loden [:reed]]

Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.92; previous revision: 1.91
done
Checking in browser/base/content/utilityOverlay.js;
/cvsroot/mozilla/browser/base/content/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.60; previous revision: 1.59
done
Checking in browser/components/places/content/editBookmarkOverlay.js;
/cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.js,v  <--  editBookmarkOverlay.js
new revision: 1.21; previous revision: 1.20
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.123; previous revision: 1.122
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.100; previous revision: 1.99
done
Checking in toolkit/mozapps/handling/content/dialog.js;
/cvsroot/mozilla/toolkit/mozapps/handling/content/dialog.js,v  <--  dialog.js
new revision: 1.13; previous revision: 1.12
done

Comment 16

[tabmix.onemen]

in today build 2008020123 after Bug 414901 landed all bookmarks of the form chrome://ietab/content/reloaded.html?url=[b]SITE ADDRESS[/b]
are not useable anymore.

IEtab extension use chrome://ietab/content/reloaded.html?url= to load the page in IE 

i get in the error console [code]
Error: Access to restricted URI denied = NS_ERROR_DOM_BAD_URI
[/code] from all the places that the code call newURI in nsIIOService with a url of the form chrome://ietab/content/reloaded.html?url=[b]SITE ADDRESS[/b]

Comment 17

[(no longer active)]

Yes, I can confirm that newURI chokes when it sees a colon in the value of the url parameter on trunk.  I tried the following code in the js console:

var ios = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService); alert(ios.newURI("chrome://blah/blah/?url=%3A", null, null).spec);

Error: uncaught exception: [Exception... "Access to restricted URI denied"  code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  location: "javascript:%20var%20ios%20=%20Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);%20alert(ios.newURI("chrome://blah/blah/?url=%253A",%20null,%20null).spec); Line: 1"]

This works fine on branch.  I'm not sure if this is caused by this bug though, and since the IO.newURI implementation which was removed used nsIIOService.newURI itself, I think the same problem must have been observable in older builds.

Here's the old IO.newURI implementation for reference:

newURI : function ScriptableIO_newURI(uri)
{
  if (!uri)
    throw Components.results.NS_ERROR_INVALID_ARG;

  var ios = Cc["@mozilla.org/network/io-service;1"].
            getService(Ci.nsIIOService);
  if (uri instanceof Ci.nsIFile)
    return ios.newFileURI(uri);
  return ios.newURI(uri, "", null);
},

Comment 18

[Bob Uliss]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9) Gecko/2008052903 Firefox/3.0

Works in RC2