Closed Bug 415218 Opened 12 years ago Closed 12 years ago

Fix uses of IO.newURI to use nsIIOService instead

Categories

(Firefox :: General, defect, P1, blocker)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta3

People

(Reporter: reed, Assigned: ehsan)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Attached patch wip (obsolete) — Splinter Review
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!
Flags: blocking-firefox3?
Priority: -- → P1
Flags: blocking-firefox3? → blocking-firefox3+
It seems Bug 415193 is another side effect because it is fixed with the backout. Do I mark as duplicate
On second thought, I might want to test first. Sorry for the spam.
Taking over.
Assignee: nobody → ehsan.akhgari
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.
Attached patch Patch (v1) (obsolete) — Splinter Review
This patch is modified a bit to fix a few problems with the patch provided by Reed.  Thanks for the original work, Reed!
Attachment #300839 - Attachment is obsolete: true
Attachment #300879 - Flags: review?(mano)
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 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).
Attachment #300879 - Flags: review?(mano) → review-
(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...
Attached patch Patch (v2) (obsolete) — Splinter Review
(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...
Attachment #300879 - Attachment is obsolete: true
Attachment #300884 - Flags: review?(mano)
Beltzner: please see comment 8.
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.
Attachment #300884 - Flags: review?(mano) → review+
Attached patch Patch (v2.1)Splinter Review
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).
Attachment #300884 - Attachment is obsolete: true
Attachment #300899 - Flags: review+
Attachment #300899 - Flags: approval1.9b3?
Comment on attachment 300899 [details] [diff] [review]
Patch (v2.1)

a=beltzner
Attachment #300899 - Flags: approval1.9b3? → approval1.9b3+
Keywords: checkin-needed
Status: NEW → ASSIGNED
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.
Flags: in-testsuite?
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 415200
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]
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);
},
Depends on: 415367
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9) Gecko/2008052903 Firefox/3.0

Works in RC2

Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.