263200 - [FIX]We use GetCurrentDoc() instead of GetOwnerDoc() and vice versa

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I just went through all uses of GetCurrentDoc()/GetOwnerDoc() and fixed up those
that were wrong (with some comments added to cases where I could not decide
which is right).

I'd really like to get this in ASAP so that I can start adding new callers of
GetCurrentDoc()/GetOwnerDoc()...

Patch coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Peter, could you check this out?

Comment 3

[Peter Van der Beken [:peterv]]

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

> Index: content/base/src/nsGenericElement.cpp
> ===================================================================

> @@ -1868,22 +1874,20 @@ nsGenericElement::HandleDOMEvent(nsPresC

> -  if (IsInDoc()) {
> -    nsIBindingManager* bindingManager = GetOwnerDoc()->GetBindingManager();
> -    if (bindingManager) {
> -      // we have a binding manager -- do we have an anonymous parent?
> -      bindingManager->GetInsertionParent(this, getter_AddRefs(parent));
> -    }
> +  nsIBindingManager* bindingManager = GetOwnerDoc()->GetBindingManager();

Note that GetOwnerDoc() can return null now, this used to be prevented by the
IsInDoc() call. I think you should check for a null ownerDoc. There's more of
these further down.

> Index: content/html/content/src/nsGenericHTMLElement.cpp
> ===================================================================

> @@ -2026,22 +2028,17 @@ void
>  nsGenericHTMLElement::GetBaseTarget(nsAString& aBaseTarget) const

> -  if (IsInDoc()) {
> -    GetOwnerDoc()->GetBaseTarget(aBaseTarget);
> -  }
> -  else {
> -    aBaseTarget.Truncate();
> -  }
> +  GetOwnerDoc()->GetBaseTarget(aBaseTarget);

Hmm, why? Also, null-check GetOwnerDoc?

> Index: content/html/content/src/nsHTMLSelectElement.cpp
> ===================================================================

>  void
>  nsHTMLSelectElement::DispatchDOMEvent(const nsAString& aName)
>  {
> -  nsCOMPtr<nsIDOMDocumentEvent> domDoc = do_QueryInterface(GetCurrentDoc());
> +  nsCOMPtr<nsIDOMDocumentEvent> domDoc = do_QueryInterface(GetOwnerDoc());
>    if (domDoc) {
>      nsCOMPtr<nsIDOMEvent> selectEvent;
>      domDoc->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(selectEvent));

Will this be ok for sXBL/XBL2?

> Index: content/svg/content/src/nsSVGElement.cpp
> ===================================================================

> @@ -454,19 +454,21 @@ NS_IMETHODIMP nsSVGElement::SetId(const 

>    nsIBindingManager *bindingManager = nsnull;
> -  if (IsInDoc()) {
> -    bindingManager = GetOwnerDoc()->GetBindingManager();
> -  }
> +  // XXXbz I _think_ this is right.  We want to be using the binding manager
> +  // that would have attached the binding that gives us our anonymous parent.
> +  // That's the binding manager for the document we actually belong to, which
> +  // is our owner doc.
> +  bindingManager = GetOwnerDoc()->GetBindingManager();

Null-check GetOwnerDoc?

> Index: content/svg/content/src/nsSVGGraphicElement.cpp
> ===================================================================

> @@ -136,19 +137,21 @@ NS_IMETHODIMP nsSVGGraphicElement::GetBB

> -  if (IsInDoc()) {
> -    bindingManager = GetOwnerDoc()->GetBindingManager();
> -  }
> +  // XXXbz I _think_ this is right.  We want to be using the binding manager
> +  // that would have attached the binding that gives us our anonymous parent.
> +  // That's the binding manager for the document we actually belong to, which
> +  // is our owner doc.
> +  bindingManager = GetOwnerDoc()->GetBindingManager();

Null-check GetOwnerDoc?

> @@ -211,19 +214,21 @@ NS_IMETHODIMP nsSVGGraphicElement::GetCT

>    nsIBindingManager *bindingManager = nsnull;
> -  if (IsInDoc()) {
> -    bindingManager = GetOwnerDoc()->GetBindingManager();
> -  }
> +  // XXXbz I _think_ this is right.  We want to be using the binding manager
> +  // that would have attached the binding that gives us our anonymous parent.
> +  // That's the binding manager for the document we actually belong to, which
> +  // is our owner doc.
> +  bindingManager = GetOwnerDoc()->GetBindingManager();

Null-check GetOwnerDoc?

> Index: content/svg/content/src/nsSVGSVGElement.cpp
> ===================================================================

> @@ -883,19 +888,21 @@ nsSVGSVGElement::GetBBox(nsIDOMSVGRect *

>    nsIBindingManager *bindingManager = nsnull;
> -  if (IsInDoc()) {
> -    bindingManager = GetOwnerDoc()->GetBindingManager();
> -  }
> +  // XXXbz I _think_ this is right.  We want to be using the binding manager
> +  // that would have attached the binding that gives us our anonymous parent.
> +  // That's the binding manager for the document we actually belong to, which
> +  // is our owner doc.
> +  bindingManager = GetOwnerDoc()->GetBindingManager();

Null-check GetOwnerDoc?

> @@ -956,19 +963,21 @@ nsSVGSVGElement::GetCTM(nsIDOMSVGMatrix 

>    nsIBindingManager *bindingManager = nsnull;
> -  if (IsInDoc()) {
> -    bindingManager = GetOwnerDoc()->GetBindingManager();
> -  }
> +  // XXXbz I _think_ this is right.  We want to be using the binding manager
> +  // that would have attached the binding that gives us our anonymous parent.
> +  // That's the binding manager for the document we actually belong to, which
> +  // is our owner doc.
> +  bindingManager = GetOwnerDoc()->GetBindingManager();

Null-check GetOwnerDoc?

> Index: content/xml/content/src/nsXMLStylesheetPI.cpp
> ===================================================================

> @@ -164,17 +164,17 @@ nsXMLStylesheetPI::GetStyleSheetURL(PRBo

>    nsIURI *baseURL;
>    nsCAutoString charset;
> -  nsIDocument *document = GetCurrentDoc();
> +  nsIDocument *document = GetOwnerDoc();
>    if (document) {
>      baseURL = document->GetBaseURI();
>      charset = document->GetDocumentCharacterSet();
>    } else {
>      baseURL = nsnull;
>    }

Will this be correct for sXBL/XBL2?
I have the same question for all the changes where we call InNavQuirksMode and
NewURIWithDocumentCharset. I don't know what we should do, so I'm interested in
your reasoning.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Note that GetOwnerDoc() can return null now

Hmm... That's unfortunate.  I'll add null-checks, but we really shouldn't have
nodes floating about without an owner doc... :(

> Hmm, why?
(for base target)

Because the base target is just a property of the document we belong to.  I
suppose I could add an assert that that code is not called when IsInDoc() is
false; I don't think there should be any callers like that...  But in the sXBL
world, links from the binding would want to use the target (if any) of the
binding document.

> Will this be correct for sXBL/XBL2?

Yes.

In brief, relative URI resolution for anonymous nodes should be relative to the
binding document, since they have no idea where the document they're being bound
to is located.  Hence it makes no sense to resolve relative to the document
they're bound to.  Resolving relative to the binding document lets the author
put resources associated with the binding in a known place relative to the
binding and then use the binding anywhere desired.

Similarly, when doing the charset thing we want to use the document that we were
parsed from initially.  That would be the ownerDocument.

Finally, for quirks, the idea is that the behavior of the XBL binding should not
change depending on what sort of document it's included into.  But past that,
there is one more serious issue.  The quirks checks would sometimes end up
deciding we're not in quirks simply because the node is not in a document.  So
in a quirks document, setting an attr and then inserting the node would behave
differently from inserting the node and then setting the attr.  Which is broken,
and in particular would have gotten in the way of the BindToTree() stuff (where
I plan to set all attrs _before_ inserting into the document).

Comment 5

[Boris Zbarsky [:bzbarsky]]

>> Index: content/html/content/src/nsHTMLSelectElement.cpp
> Will this be ok for sXBL/XBL2?

It should be; createEvent doesn't really care which doc it's called on much. 
This change just lets these events be dispatched for selects which are not in
the document (which I think is desirable).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 7

[Peter Van der Beken [:peterv]]

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

Ok, I'm convinced. Note that there will not be many nodes without an
ownerDocument.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in