Last Comment Bug 324464 - We should expose nsIURI getters for Node.baseURI and Document.documentURI
: We should expose nsIURI getters for Node.baseURI and Document.documentURI
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on: 326767
Blocks: 327244 342485
  Show dependency treegraph
 
Reported: 2006-01-23 16:13 PST by Darin Fisher
Modified: 2008-03-10 18:58 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (14.53 KB, patch)
2006-05-04 19:31 PDT, Boris Zbarsky [:bz] (still a bit busy)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Updated to jst's comments (14.63 KB, patch)
2006-06-22 18:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description Darin Fisher 2006-01-23 16:13:38 PST
We should expose nsIURI getters for Node.baseURI and Document.documentURI

Right now, an extension needs to get the baseURI strings, and the document's input encoding to properly construct a nsIURI.  This is silly since Gecko already has the nsIURI hanging around in its internals.
Comment 1 Christian :Biesinger (don't email me, ping me on IRC) 2006-01-23 16:30:47 PST
this would be useful for browser code too, compare bug 262222 comment 27
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2006-01-23 19:19:45 PST
So what we need is a way to get these without exposing stuff to content (in particular without polluting the content-visible namespace)... Perhaps we should toss more stuff onto nsIDOMWindowUtils?
Comment 3 Darin Fisher 2006-01-23 21:49:25 PST
Hmm.. what about documents that do not have an associated window?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2006-01-23 21:56:02 PST
You mean like XMLHttpRequest ones?  ;)  Good catch.

I guess we either need a service or nsIDOMDocumentUtils or something...  jst, what do you think?
Comment 5 Darin Fisher 2006-01-23 23:04:24 PST
> You mean like XMLHttpRequest ones?  ;)  Good catch.

yeah, and ones created from DOMImplementation.  XForms is a good example of an extension that works with such documents in C++.  It uses nsIContent::GetBaseURI and nsIDocument::GetDocumentURI.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-01-23 23:06:25 PST
We could put the methods on an interface implemented by the various classes (using a tearoff), and simply not list the interface in nsIClassInfo
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2006-01-23 23:17:22 PST
Hmm...  I was worried that what sicking suggests would lead to the content-visible wrapper exposing the property once the chrome uses them, but with XPCNativeWrapper done in C++, that may no longer be an issue...  If so, that's totally the way to go.

Anyone know what our behavior would be there at this point?
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-01-24 21:22:27 PST
Is the wrapper the only thing that would save us? If so it might be safer to go with a separate utility class. But I thought that we might have additional security checks that prevented you from calling methods on interfaces that are not in nsIClassInfo.

Or would the methods still show up as properties on the node once someone has QI'ed to the new interface? But just not callable?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2006-02-18 15:49:38 PST
> Or would the methods still show up as properties on the node once someone has
> QI'ed to the new interface? But just not callable?

Right.

I had another idea last night -- using a noscript interface (or an interface with noscript methods) and changing classinfo to handle JS access to the properties.  Classinfo could do IsCallerChrome checks in resolve and get hooks.

What I can't decide is whether this is a really good idea or a really nasty hack... I guess if we need to support this in general in XPConnect it's more of the latter, but do we need that?

jst, sicking, brendan?  Thoughts?
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-02-19 00:21:07 PST
So your idea is when chrome access .baseURI on a node it'll get an nsIURI object back, but when content does it it'll get a string?

It seems to me like asking for trouble when people get something they didn't expect. Though i'm not sure how big of a problem that is. Especially if we make nsIURI.toString() return the uri as a string.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2006-02-19 12:27:18 PST
I was going to name the methods on this new interface something that doesn't collide with the DOM methods.  Say gkBaseURI and gkNodePrincipal if we want to be really safe?  Or just nodeBaseURI and nodePrincipal?
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2006-02-19 12:30:53 PST
Also, it might be possible to just mark an entire interface as "chrome only" or something.  Like bug 326767 suggests.  And then put magic in FindMember or something?

If we can do that, great.  But I'd really like to get this bug fixed so we can do work it blocks for 1.9, so if bug 326767 is a major production and would take a long time, maybe should do what I suggest in comment 9.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2006-05-04 12:30:52 PDT
The suggestion from comment 9 was brought up again today.  I think we should just do that.
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-04 18:56:41 PDT
I'm fine with doing comment 9 if we think this is going to be a fairly uncommon thing to do. It'd be nice to have a better solution if we end up doing this a lot though, but that's a bridge we could cross at that point.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2006-05-04 19:19:03 PDT
Yeah.  This is blocking work I want to do on the security manager APIs, so I'm not really going to wait for a future something better, methinks.... I'm just doing the classinfo end for now; I can't think of a sane (IDL) interface to put the stuff on without adding vtable pointers.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2006-05-04 19:31:30 PDT
Created attachment 220872 [details] [diff] [review]
Proposed patch

This exposes JS-only getters for Node.nodePrincipal, Node.baseURIObject and Document.documentURIObject.  Sadly, with this patch chrome script can't access the content-set values of those properties, and any script that does cause these properties to be resolved on an object will prevent them being set thereafter.  So if chrome messes with these properties on a wrappedJSObject of an XPCNativeWrapper, the site won't be able to set them after that.  Or if the site sets them first, the chrome messing with them will leak the objects to content... The point is, chrome should not touch these properties on untrusted (not wrapped) content objects.  As long as we hold to that, life is ok.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2006-06-21 16:46:41 PDT
Comment on attachment 220872 [details] [diff] [review]
Proposed patch

Hmm, interesting approach :) Looks like this would work, a couple of minor comments below...

- In nsDOMClassInfo::ShutDown():

+  sAdd_id             = JSVAL_VOID;
+  sAdd_id             = JSVAL_VOID;
+  sTags_id            = JSVAL_VOID;
...

The second sAdd_id there should be sAll_id. Thanks for fixing this!

- In nsDocumentSH::GetProperty():

+  if (id == sDocumentURIObject_id && IsPrivilegedScript()) {
[...]
+    nsresult rv = WrapNative(cx, obj, uri, NS_GET_IID(nsIURI), vp,
+                             getter_AddRefs(holder));
+    return NS_FAILED(rv) ? rv : NS_SUCCESS_I_DID_SOMETHING;
+
+  }

Loose the empty line after return, or move it just above the return line.

r+sr=jst
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2006-06-21 19:34:38 PDT
Comment on attachment 220872 [details] [diff] [review]
Proposed patch

Brendan, are you OK with this?  I recall you had some concerns, but I can't recall whether I addressed them...
Comment 19 Brendan Eich [:brendan] 2006-06-22 18:21:22 PDT
(In reply to comment #18)
> (From update of attachment 220872 [details] [diff] [review] [edit])
> Brendan, are you OK with this?  I recall you had some concerns, but I can't
> recall whether I addressed them...

Nah, worse is better!

/be
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2006-06-22 18:44:24 PDT
Created attachment 226734 [details] [diff] [review]
Updated to jst's comments
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2006-06-22 18:47:14 PDT
Checked in.
Comment 22 Eric Shepherd [:sheppy] 2007-10-09 17:28:32 PDT
I've documented the documentURIObject method since DOM:document is already documented; the other two methods are listed only in the Fx3 for developers page and will be documented properly when DOM:node is documented.

http://developer.mozilla.org/en/docs/DOM:document.documentURIObject
http://developer.mozilla.org/en/docs/Firefox_3_for_developers#DOM

Marking this as dev-doc-complete and filing a new bug requesting documentation for DOM:node; the full documentation will fall into place naturally when DOM:node is written up.
Comment 23 Nickolay_Ponomarev 2007-10-26 03:25:39 PDT
Let me check if I understand this correctly before I tweak the documentation. The new properties are chrome-only (per comment 2), right? (That's not covered in the docs)

And chrome code must *not* touch wrappedDoc.wrappedJSObject.documentURI and similar (per comment 16), right? The docs say the opposite.

This makes no sense to me, care to clarify?
+    // Is there a better error we could use here?  We don't want privileged
+    // script that can read this property to set it, but _do_ want to allow
+    // everyone else to.
If documentURIObject and others are chrome only, who is "everyone else" we want to let setting it? What does setting it mean anyway?

I guess I'm totally confused by this (and that the patch didn't add any documentation about the new bits to the code doesn't help) and as far as I can see, the documentation is very confused too.
Comment 24 Nickolay_Ponomarev 2007-10-26 03:28:21 PDT
Oh and we do need documentation for at least nodePrincipal, since there's no other way I can see to get any information about it (no human-readable comments in the code).
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2007-10-26 12:16:07 PDT
> The new properties are chrome-only (per comment 2), right?

No.  The new properties are only available in script that has UniversalXPConnect privileges.  That includes chrome, but is not limited to chrome.

> And chrome code must *not* touch wrappedDoc.wrappedJSObject.documentURI 

That is correct.

> This makes no sense to me, care to clarify?

Sure.  The behavior I was trying to achieve is the following:

1)  If the script has UniversalXPConnect, the three properties in question
    (nodePrincipal, baseURIObject, documentURIObject) are readonly properties
    that hand back the relevant object.  Attempting to set them throws,
    since they are readonly.
2)  If the script doesn't have UniversalXPConnect then these three properties
    are like any other "expando" property (undefined to start with, you can set
    them to some value if you want, when you get them you get whatever value
    you set them to, etc.).

It's a somewhat confusing model; I'm a little sad that this is the best we can do without introducing web compat risks.

Perhaps the comment would have been better written as:

    // We don't want privileged script that can read this property to set it,
    // but _do_ want to allow everyone else to set a value they can then read.
    //
    // XXXbz Is there a better error we could use here?

Is that clearer?  If so, I'll change it accordingly.

As far as nodePrincipal goes, it returns an nsIPrincipal object representing the current security context of the node.  Let me know if you need me to expand on that somehow?
Comment 26 Nickolay_Ponomarev 2007-10-26 18:22:32 PDT
Yeah, the new comment is clearer, thanks. In the hindsight, I could have figured out the intentions from the old comment, but it wasn't obvious, since the read-onlyness of the new properties for UniversalXPConnect code wasn't mentioned anywhere that I could see.

As for nodePrincipal, *I* figured that it returns that from the code, but it still should be documented if we think it may be useful to our developer community (preferably along with clear usage examples).

I don't know when I'll get to updating the docs based on the information you posted, hopefully this weekend.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2007-10-26 18:24:53 PDT
> Yeah, the new comment is clearer, thanks. 

OK.  I've updated the comment in the code.

> but it still should be documented

Oh, absolutely.  Thank you for documenting this stuff!  Please let me know if there's something I can do to help!
Comment 28 Nickolay_Ponomarev 2007-11-01 09:33:48 PDT
I moved the mention of these new getters from the "For web site and application developers" to "Other platform functionality", since this shouldn't affect web developers:
http://developer.mozilla.org/en/docs/Firefox_3_for_developers#Other_platform_functionality

Also updated http://developer.mozilla.org/en/docs/DOM:document.documentURIObject and the docs at http://developer.mozilla.org/en/docs/DOM:document

Didn't add documentation of the new getters on Node; they should go to http://developer.mozilla.org/en/docs/DOM:element like the rest of the Node stuff, until we finally get to organizing the DOM reference in the proper way.
Comment 29 Eric Shepherd [:sheppy] 2008-03-07 18:10:46 PST
Are baseURIObject and nodePrincipal the getters on Node that still need to be documented?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2008-03-07 18:28:30 PST
Yep.
Comment 31 Eric Shepherd [:sheppy] 2008-03-07 18:32:25 PST
I figured that out to my satisfaction about 3 minutes after asking. :)

OK, I've added baseURIObject and nodePrincipal to:

http://developer.mozilla.org/en/docs/DOM:element

And they are documented here:

http://developer.mozilla.org/en/docs/DOM:element.baseURIObject
http://developer.mozilla.org/en/docs/DOM:element.nodePrincipal

I'm marking this as doc-complete; if there's an issue with these docs, please re-open or fix them. :)
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2008-03-07 18:36:05 PST
Note that these properties are on all Nodes, not just on Elements.  Probably doesn't matter for a lot of uses, but might for some...
Comment 33 Eric Shepherd [:sheppy] 2008-03-07 18:37:58 PST
Yeah, I realize that, but we don't actually have Node documentation yet. :)
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2008-03-07 19:13:20 PST
Hmm.  I just read the documentation, and it doesn't seem right (or perhaps I'm misreading the "Availability" column).

The properties are there on all nodes (HTML, XUL, SVG, MathML, whatever), but only if the script that's asking for them has UniversalXPConnect privileges.  I don't see an obvious way to describe that in the context of this page.

Also, if we're going to put these on Element, might as well put them on Document too, until we have Node documentation.
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2008-03-07 19:14:37 PST
Oh, and baseURIObject is the base URI for the _node_, not for the _document_...
Comment 36 Eric Shepherd [:sheppy] 2008-03-10 08:42:43 PDT
I really need to do some studying about the relationship between nodes, documents, and elements. :)

OK, I've updated this documentation and added these two properties to the DOM:document as well:

http://developer.mozilla.org/en/docs/DOM:document

If this looks satisfactory, mark this as dev-doc-complete, please.
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2008-03-10 09:40:26 PDT
Basically, Document, Element, and Attr all inherit from Node.  That's the relationship.  ;)

The text looks good now, but the "Availability" thing is still wrong, as far as I can tell...
Comment 38 Eric Shepherd [:sheppy] 2008-03-10 12:08:22 PDT
I've tweaked the table at

http://developer.mozilla.org/en/docs/DOM:element

So that it says "All (with UniversalXPConnect privileges)" for availability, and added "New in Firefox 3" boxes to these two getters.
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2008-03-10 18:58:03 PDT
That looks great.  Thank you!

Note You need to log in before you can comment on or make changes to this bug.