We should expose nsIURI getters for Node.baseURI and Document.documentURI

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Darin Fisher, Assigned: bz)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla1.9alpha1
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
this would be useful for browser code too, compare bug 262222 comment 27
(Assignee)

Comment 2

12 years ago
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?
(Reporter)

Comment 3

12 years ago
Hmm.. what about documents that do not have an associated window?
(Assignee)

Comment 4

12 years ago
You mean like XMLHttpRequest ones?  ;)  Good catch.

I guess we either need a service or nsIDOMDocumentUtils or something...  jst, what do you think?
(Reporter)

Comment 5

12 years ago
> 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.
We could put the methods on an interface implemented by the various classes (using a tearoff), and simply not list the interface in nsIClassInfo
(Assignee)

Comment 7

12 years ago
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?
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?
(Assignee)

Updated

12 years ago
Blocks: 327244
(Assignee)

Comment 9

12 years ago
> 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?
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.
(Assignee)

Comment 11

12 years ago
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?
(Assignee)

Comment 12

12 years ago
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.
(Assignee)

Updated

12 years ago
Depends on: 326767
(Assignee)

Comment 13

11 years ago
The suggestion from comment 9 was brought up again today.  I think we should just do that.
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.
(Assignee)

Comment 15

11 years ago
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.
(Assignee)

Comment 16

11 years ago
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.
Attachment #220872 - Flags: superreview?(jst)
Attachment #220872 - Flags: review?(jst)
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
Attachment #220872 - Flags: superreview?(jst)
Attachment #220872 - Flags: superreview+
Attachment #220872 - Flags: review?(jst)
Attachment #220872 - Flags: review+
(Assignee)

Comment 18

11 years ago
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...
Attachment #220872 - Flags: review?(brendan)
(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
(Assignee)

Comment 20

11 years ago
Created attachment 226734 [details] [diff] [review]
Updated to jst's comments
Attachment #220872 - Attachment is obsolete: true
Attachment #220872 - Flags: review?(brendan)
(Assignee)

Comment 21

11 years ago
Checked in.
Assignee: general → bzbarsky
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 342485
(Assignee)

Updated

11 years ago
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete

Comment 23

10 years ago
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.
Keywords: dev-doc-complete → dev-doc-needed

Comment 24

10 years ago
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).
(Assignee)

Comment 25

10 years ago
> 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

10 years ago
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.
(Assignee)

Comment 27

10 years ago
> 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

10 years ago
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.
Are baseURIObject and nodePrincipal the getters on Node that still need to be documented?
(Assignee)

Comment 30

10 years ago
Yep.
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. :)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 32

10 years ago
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...
Yeah, I realize that, but we don't actually have Node documentation yet. :)
(Assignee)

Comment 34

10 years ago
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.
Keywords: dev-doc-complete → dev-doc-needed
(Assignee)

Comment 35

10 years ago
Oh, and baseURIObject is the base URI for the _node_, not for the _document_...
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.
(Assignee)

Comment 37

10 years ago
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...
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.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 39

10 years ago
That looks great.  Thank you!
You need to log in before you can comment on or make changes to this bug.