209573 - [FIX]Move GetBaseURI up to nsIContent

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

nsIHTMLContent has a GetBaseURL thing that uses magic attributes in HTML...
nsIXMLContent has something similar that uses xml:base.  Plus there is the
document base URI.  Most callers don't do a very good job of looking at all of
these when they need a base URI, nor should they have to.  Imo, nsIContent
should just have a GetBaseURI() function that does the right thing depending on
the type of content, etc.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

I'll drink to that

Comment 2

[Peter Lubczynski]

I think plugins may needs this too. OBJECT tag has a CODEBASE attribute plus
calls to NPP_Get/Post/URL need to take the document's base into account.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Peter, do you think that <object> should have its own impl of GetBaseURI that
takes the codebase into account?  Are there times when URIs (other than codebase
itself) would need to be resolved relative to the document base or xml:base for
an <object> rather than relative to the codebase?

Comment 4

[Peter Lubczynski]

yes, to both questions. There are times when codebase shouldn't be trusted and
those are based on what kind of content the tag will host. For example, codebase
is important for Java but meaningless for Active-X controls.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ok.  But for any given object, all uris should be resolved either relative to
codebase or not, correct?  What I'd like to avoid is creating a GetBaseURI that
takes codebase into account for Java, then discovering that someone needs to
load something off a Java <object> from a URL _not_ relative to codebase...
because there will be no way to do it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

This has some junk from bug 166996 mixed in -- I'm touching the same exact code
and there is really no way I can separate the two patches out from each
other...

I'll be posting a cleaned-up patch once bug 166996 is checked in.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126726 [details] [diff] [review]
Patch

Chris, if you have time to look at this in this state, that would be great.  If
not, bug 166996 is clearly a bigger priority for me...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Better patch

This is merged to tip (post-checkin-of-166996).  There's a change in editor
which is not, strictly speaking, related to this bug, but it was near a
GetBaseURL caller and the code was ugly....

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 126827 [details] [diff] [review]
Better patch

- In nsIContent.h:

+  /**
+   * Get the base URL for any relative URLs within this piece
+   * of content. Generally, this is the document's base URL,
+   * but certain content carries a local base for backward
+   * compatibility.

There's more than backwards compatibility that plays in here, especially
xml:base, so you might want to mention that here too.

- In nsGenericElement::GetBaseURL():

+      rv = securityManager->CheckLoadURI(parentBase, ourBase,
+			  nsIScriptSecurityManager::STANDARD);

I realize you didn't make this up, but this seems like bad design to me. IMO
there shouldn't be any reason to do a security check when requesting the base
URI of anything, the security check should be done either when setting the base
URI, or when using the base URI, not when requesting it. Thoughts?

- In nsXULElement::GetBaseURL():

+  // XXX TODO, should share the impl with nsGenericElement

Wanna create a static method somewhere (nsGenericElement, nsContentUtils, ...)
where the code for this could live and make the appropriate classes call that
function?

sr=jst

Comment 10

[Boris Zbarsky [:bzbarsky]]

> There's more than backwards compatibility that plays in here

Indeed.  Will update.

> IMO there shouldn't be any reason to do a security check when requesting the
> base URI of anything, the security check should be done either when setting
> the base URI, or when using the base URI, not when requesting it. Thoughts?

Well... There is no way to do it when "setting", since xml:base is cumulative
from the top of the tree down and thus affected by DOM manipulation.  In fact,
you could argue that what I do _is_ doing the "setting" check (in the absence of
DOM manipulation, it would have exactly the same effect as doing the check while
parsing).

As for doing the check when using the baseURI, this has two drawbacks: 1) every
single caller of GetBaseURL would have to do the check, as far as I can tell,
and 2) There is no way to gracefully handle the following markup (served from
http://foo/ :

<root xml:base="http://bar/">
  <kid xml:base="file:///home/">
    <html:img src="test.gif" />
  </kid>
</root>

In my opinion, in this scenario the image should be loaded from
"http://bar/test.gif" (essentially a security check on the second xml:base being
set at all), just like it would be if you had an HTML document at "http://foo/"
that had
<base href="file:///home/">.  However, if the caller of GetBaseURL does the
check, their only options are to use "file:///home/" or "http://foo/" for the
base -- they have no knowledge of "http://bar/".

> Wanna create a static method somewhere

If we're sure we have no plans to ever make nsXULElement inherit from
nsGenericElement, sure.  Otherwise, I'd rather leave this as-is for now...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Note to self -- there is a BaseURL caller in the scriptLoader that should call
it off nsIContent, not nsIDocument...

If people want, I can post the additional patch for that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Include that, and a cssloader change

Since review has not happened yet... ;)

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 126960 [details] [diff] [review]
Include that, and a cssloader change

> NS_IMETHODIMP
> nsGenericElement::GetBaseURL(nsIURI** aBaseURL) const
> {
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  nsCOMPtr<nsIDocument> doc;
>+  GetDocument(getter_AddRefs(doc));

How about |nsCOMPtr<nsIDocument> doc = mDocument;|?

>+
>+  if (!doc && mNodeInfo) {
>+    mNodeInfo->GetDocument(getter_AddRefs(doc));
>+  }



> nsresult
> nsGenericHTMLElement::GetBaseURL(nsIURI** aBaseURL) const
> {
...
>+  if (mNodeInfo->NamespaceEquals(kNameSpaceID_None)) {
>+    if (doc) {
>+      return doc->GetBaseURL(aBaseURL);
>+    } else {

Else after return.

>+      *aBaseURL = nsnull;
>+      return NS_OK;
>+    }
>+  }



>Index: editor/libeditor/html/nsHTMLEditor.cpp
>@@ -5979,20 +5979,17 @@ nsHTMLEditor::ParseStyleAttrIntoCSSRule(
> 
>   nsCOMPtr<nsIDocument> doc (do_QueryInterface(domdoc));
>   if (!doc)
>     return NS_ERROR_UNEXPECTED;
>   nsCOMPtr <nsIURI> docURL;
>   doc->GetBaseURL(getter_AddRefs(docURL));
>   nsCOMPtr<nsICSSParser> css;
>   nsCOMPtr<nsIStyleRule> mRule;
>-  nsComponentManager::CreateInstance(kCSSParserCID,
>-                                     nsnull,
>-                                     NS_GET_IID(nsICSSParser),
>-                                     getter_AddRefs(css));
>+  css = do_CreateInstance(kCSSParserCID);

Declare the var while assigning instead of two statements?



>Index: content/html/style/src/nsCSSLoader.h

>   nsresult CreateSheet(nsIURI* aURI,
>+                       nsIContent* aLinkingContent,
>                        PRUint32 aDefaultNameSpaceID,
>                        PRBool aSyncLoad,
>                        StyleSheetState& aSheetState,
>                        nsICSSStyleSheet** aSheet);

Add a comment to the above method which notes when aLinkingContent can be null
and when it shouldn't?	I think your passing of null in
CSSLoaderImpl::InternalLoadAgentSheet() is correct, but also please double
check that it doesn't trigger the assert you placed into the implementation of
CreateSheet(). 

The rest looks fine.  r=caillon

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: With those changes

Comment 15

[Boris Zbarsky [:bzbarsky]]

Checked in at "07/02/2003 19:45 PDT".