Closed Bug 324464 Opened 19 years ago Closed 18 years ago

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

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: darin.moz, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

Updated to jst's comments
14.63 KB, patch
Details | Diff | Splinter Review 
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
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?
Hmm.. what about documents that do not have an associated window?
You mean like XMLHttpRequest ones?  ;)  Good catch.

I guess we either need a service or nsIDOMDocumentUtils or something...  jst, what do you think?
> 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
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?
Blocks: 327244
> 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.
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?
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.
Depends on: 326767
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.
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.
Attached patch Proposed patch (obsolete) — — Splinter Review 
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+
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

        Attachment #220872 -
        Attachment is obsolete: true

        Attachment #220872 -
        Flags: review?(brendan)

    
    

    
  
Checked in.
Assignee: general → bzbarsky
Target Milestone: --- → mozilla1.9alpha

    
  
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

    
  
Blocks: 342485

    
  
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

    
    

    
  
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

    
    

    
  
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).

    
    

    
  
> 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?

    
    

    
  
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.


    
    

    
  
> 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!

    
    

    
  
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?

    
    

    
  
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

    
    

    
  
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. :)

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
That looks great.  Thank you!
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: