132300 - Need a better way to bootstrap an XPathEvaluator

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Currently we support |var foo = new XPathEvaluator();| but the DOM XPath WD
specifies "It is expected that the XPathEvaluator interface will be implemented
on the same object which implements the Document interface in an implementation
which supports the XPath DOM module.". I have a solution with a tear-off and
dynamic QI and DOMCI which makes document an XPathEvaluator when the
Transformiix module is installed. Patch coming up later today. Jst: do we want
this for 1.0?

Comment 1

[Johnny Stenback (:jst)]

IMO we do want this for mozilla1.0, if we don't take this we'll force users of
these standard API's to use non-standard mechanisms for accessing these API's.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 3

[Peter Van der Beken [:peterv]]

I've created a cached tear-off for nsDocument which implements
nsIDOMXPathEvaluator. The document creates this tear-off when it gets QI'ed to
nsIDOMXPathEvaluator and we know that there is an implementation for the XPath
DOM module. We detect that by querying for a ContractID when loading the content
module. The tear-off forwards AddRef and Release to the document, and QI for
interfaces other than nsIDOMXPathEvaluator too. When the document is destroyed
the tear-off gets deleted. The tear-off creates and holds an
nsIDOMXPathEvaluator and forwards calls to it. For the DOMCI, I do the same
check for the implementation and add the IID for nsIDOMXPathEvaluator at the end
if it's there. If it's not there, I just add two nsnull's to the end of the
array instead of that IID and one nsnull. I think this approach is sound, though
a bit strange. It does work, document.evaluate for example works and I saw no
leaks. I would like to change the ContractID to something implementation-neutral
(right now it uses the Transformiix one). Ideas for the name of the ContractID
welcome.
/me waits for jst to tell him that he's insane.

Comment 4

[Ray Whitmer]

How does this work in Javascript, is the question.  One would expect to just be
able to call the evaluate method on document (because the level 3 getInterface
method is not here yet, and even with this, I am not sure how it works with
Javascript).

Does the Javascript user have to explicitly call QuaryInterface?

Comment 5

[Peter Van der Beken [:peterv]]

No, you just do document.evaluate(...) or document.createExpression(...).
getInterface won't work anyway since it returns a Node and XPathEvaluator
doesn't inherit from Node.

Comment 6

[Ray Whitmer]

Comment on attachment 75273 [details] [diff] [review]
v1

r=rayw.

It looks good to me.  No major problems found.	I only found one issue that
affects behavior (responding to feature version).  Any of these issues could be
logged as seperate bugs and not block the checkin.
Here follow my notes:

>>>>>>> nsDOMClassInfo.cpp:

1. Assumes that GetClassInfo is called only after XPath module is registered.
During an installation run it seems remotely possible it would not be true,
but this doesn't seem like a big  risk or problem if it does happen (because
it will work the nest time).  It might be good to make a note of it, unless
you believe it cannot occur.

2. The name of the new macro would be really wrong if someone decides some
document types shouldn't have XPaths.  I might make it more obvious what it
does and call it DOM_CLASSINFO_MAP_END_WITH_XPATH.

3. I usually put contractid's such as the one for the xpath-evaluators in the
.idl file containing the primary interface, following the example set by
others, and especially since it is used in multiple places, this might be
the right thing to do.

1 and 3 occur in multiple files.

>>>>>>> nsContentModule.cpp:

gHaveXPathDOM should be in a header file since it is shared, or so I have
been told.  This applies to multiple files.

>>>>>>> nsDocument.h

You did not use nsCOMPtr for mXPathDocument (meaning you later had to
explicitly delete).  Presumably this is because it gave you some trouble.
It might give less trouble if you made it of type nsIXPathEvaluator, since
it is not clear why you need to keep the pointer to the fully-derived
type.

>>>>>>> nsDocument.cpp

I believe you could use NS_DECL_ISUPPORTS_INHERITED (I believe it is intended
for this circumstance) to declare QueryInterface, AddRef, Release, etc.

>>>>>>> nsGenericElement.cpp

When checking the version string, you should, I believe, also
check for an empty string because according to the DOM spec,
users may ask without a version number (and are quite likely to
if there is only one version released so far).

Comment 7

[Ray Whitmer]

Attachment: v2

We really need at least the one behavioral change, so here it is.

Comment 8

[Ray Whitmer]

Comment on attachment 75759 [details] [diff] [review]
v2

r=rayw.  See previous comments.

Comment 9

[Peter Van der Beken [:peterv]]

>>>>>>>> nsDOMClassInfo.cpp:
> 
> 1. Assumes that GetClassInfo is called only after XPath module is registered.
> During an installation run it seems remotely possible it would not be true,
> but this doesn't seem like a big  risk or problem if it does happen (because
> it will work the nest time).  It might be good to make a note of it, unless
> you believe it cannot occur.

It seems extremely unlikely it could occur.

> 2. The name of the new macro would be really wrong if someone decides some
> document types shouldn't have XPaths.  I might make it more obvious what it
> does and call it DOM_CLASSINFO_MAP_END_WITH_XPATH.

Agreed. DOM_CLASSINFO_DOCUMENT_WITH_XPATH_MAP_END seems better but is very long.
Jst?

> 3. I usually put contractid's such as the one for the xpath-evaluators in the
> .idl file containing the primary interface, following the example set by
> others, and especially since it is used in multiple places, this might be
> the right thing to do.

I haven't done this yet for two reasons. 1) I need to decide on the final
contractID for an XPathEvaluator and 2) I had heard contractids should not be
put in .idl files. Jst?

> 1 and 3 occur in multiple files.
> 
>>>>>>>> nsContentModule.cpp:
> 
> gHaveXPathDOM should be in a header file since it is shared, or so I have
> been told.  This applies to multiple files.

It is marked as extern so I think this should be ok.

>>>>>>>> nsDocument.h
> 
> You did not use nsCOMPtr for mXPathDocument (meaning you later had to
> explicitly delete).  Presumably this is because it gave you some trouble.
> It might give less trouble if you made it of type nsIXPathEvaluator, since
> it is not clear why you need to keep the pointer to the fully-derived
> type.

No way I can use an nsCOMPtr, since that would create a circular dependency
between the document and the tear-off. Now, the ownership model is that the
document keeps the evaluator alive as long as it (the document) is alive, and
due to the forwarded AddRef and Release from the tear-off to the document, the
document stays alive as long as someone wants a reference to the tear-off.

>>>>>>>> nsDocument.cpp
> 
> I believe you could use NS_DECL_ISUPPORTS_INHERITED (I believe it is intended
> for this circumstance) to declare QueryInterface, AddRef, Release, etc.

Hmm, I guess so. I avoided it because of the _INHERITED suffix, there's no
inheritence involved here.

Comment 10

[Ray Whitmer]

As I said before, for anything non-trivial, just get a superreview and land the
thing first  rather than debating too much right now and log bugs.  All we are
talking about is stylistic, not behavioral.

[...]
>Agreed. DOM_CLASSINFO_DOCUMENT_WITH_XPATH_MAP_END seems better but is very long.
>Jst?

You added back _DOCUMENT which was part of what I objected to (since it is not
clear that all documents will implement xpath or all sources of xpaths will be
documents) and made it too long again.  My suggestion was one character longer
than your original one.

[...]
>I haven't done this yet for two reasons. 1) I need to decide on the final
>contractID for an XPathEvaluator and 2) I had heard contractids should not be
>put in .idl files. Jst?

ContractIDs are significantly higher-level than implementation but
slightly-lower than interfaces (a collection of interfaces).  This partially
depends upon whether the final name has "transformix" in it or not -- i.e.
whether it makes transformix-specific promises that another XPath DOM
implementation would be likely to break.  So you are probably right as you have
defined this, although an .h file would be nice since it is repeated so many times.

>> 1 and 3 occur in multiple files.
>> >>>>>>>> nsContentModule.cpp:
>> > gHaveXPathDOM should be in a header file since it is shared, or so I have
>> been told.  This applies to multiple files.
>
>It is marked as extern so I think this should be ok.

I made the same argument once and was told adamantly that any extern should be
in a header file.  As you say, ask jst, his opinion may be different.

>No way I can use an nsCOMPtr, since that would create a circular dependency

Sorry, my mistake.  I haven't done this style of aggregation before.  Were that
not the case the delete would have been more of a problem than stylistic.  It
looks correct as it is.

>Hmm, I guess so. I avoided it because of the _INHERITED suffix, there's no
inheritence involved here.

As I looked, I saw two things that made me think it was appropriate: first, a
comment that did not mention inheritance saying this is the way to declare those
three methods, second, examples using it that were not inheriting.  Perhaps the
creator thought it would be mostly used for inheritance, just as you thought
your new macro would be exclusively used on documents.  I won't insist if you
feel strongly.

Comment 11

[Johnny Stenback (:jst)]

I'd say go with the macro name DOM_CLASSINFO_MAP_END_WITH_XPATH, and as for
where to put the contractID, keep that out of the nsIDOM file, we'll eventually
want to freeze that file, and we don't want to freeze the contractID with the
interface. As for gHaveXPathDOM, if there's a good non-public header file you
can put it in, go for it, but if not, leave it as is. It's no big deal as long
as the linker is happy.

One thing we should check with this is that mozilla still functions if the xpath
stuff is registerd but the component isn't availiable any more. I.e. install n'
run an optimized build, exit, remove the dll where this xpath code lives and
start mozilla again. As long as it runs n' window.document is accessable on a
webpage we're ok. If not, we're in trouble.

Comment 12

[Ray Whitmer]

Based upon a review of the code, the scenario that Johnny says to test is one
that I believe will probably fail, so if it is important, then "we are in trouble"..

gHaveXPathDOM is set acording to a simple check of the component registry, and
the component registry does not remove components when they didappear (or did
not when I last reviewed that code).

Not only will the interface get erroneously added after it has disappeared, but
also, the code which instandiates transformix, which looks harmless if
gHaveXPathDOM is false, looks like it will throw an exception if it is true and
the component cannot be created.

I believe that there are other optional components in Mozilla that once they
have been registered in the category manager or by contract id will cause
Mozilla to fail if they are then removed.  Is this really something we need?  If
so, depending upon how robust it needs to be, we may impact startup time trying
to check for whether the module can really be activated before adding the
interface.  If we didn't insist on removing the interface, then it could be made
a bit more robust -- just calls to the interface would raise exceptions.

Comment 13

[Axel Hecht]

I guess
+      NS_ENSURE_TRUE(evaluator, NS_ERROR_OUT_OF_MEMORY);
should test for the error code better.
If we know the exact nsresult we get for a removed component, we could unset
gHaveXPathDOM.
I don't think that we should be picky about interface flattening on a broken
installation, though.

Comment 14

[Ray Whitmer]

As I read the code, the first call to hasFeature("XPath") will erroneously tell
the script that it has the functionality in question.  Then, every time the code
tries to access the features in question, there will be some kind of exception.

Clearing the flag would make future calls to hasFeature("XPath) return false,
but the flattened interface is still there so attempts to access parts of that
would continue to fail.

It is not clear to me that it is a good idea to have the document behave the
first time it is retrieved from subsequent times in the same session (after the
flag has been cleared so that hasFeature now returns false.
You could make hasFeature("XPath") try to create the necessary component before
reporting true.  Arguably this only adds overhead to the case where it is being
used., and make the error returned on failure to create the component (the first
time) indistinguishable from the simple not found case, after logging a
high-quality error message.  This way, you get a more-accurate hasFeature
response and indistinguishable results from one time to the next.

Comment 15

[Axel Hecht]

how about adding the contractID to 
http://lxr.mozilla.org/seamonkey/source/dom/public/nsDOMCID.h
?
Now for some cool values of the ContractID, it ain't transformiix special,
it's a DOM spec, so I'd go for something beneath @mozilla.org/dom. I would go
directly for version ;3, or include the level as ?level=3. 
"@mozilla.org/dom/xpath-evaluator;3" or 
"@mozilla.org/dom/xpath-evaluator;1?level=3"
macroname could something like NS_XPATH_EVALUATOR3_CONTRACTID

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v3

With this one hasFeature always returns the right answer, the only minor
annoyance is that the flattened interfaces will include nsIXPathEvaluator even
when the module has been removed. I've also removed the interface from the
DOMCI for nsXULDocument because it's the only one that doesn't inherit from
nsDocument.

Comment 17

[Axel Hecht]

Comment on attachment 78053 [details] [diff] [review]
v3

>Index: mozilla/content/base/src/nsDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsDocument.cpp,v
>retrieving revision 3.372
>diff -u -3 -r3.372 nsDocument.cpp
>--- mozilla/content/base/src/nsDocument.cpp	5 Apr 2002 05:41:37 -0000	3.372
>+++ mozilla/content/base/src/nsDocument.cpp	6 Apr 2002 16:37:44 -0000
<...>
>@@ -592,6 +626,22 @@
>   NS_INTERFACE_MAP_ENTRY(nsIDOM3Node)
>   NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDocument)
>+  if (aIID.Equals(NS_GET_IID(nsIDOMXPathEvaluator)) &&
>+      (!gCheckedForXPathDOM || gHaveXPathDOM)) {
>+    if (!mXPathDocument) {
shouldn't this be
    if (!gCheckedForXPathDOM && !mXPathDocument) {
>+      nsCOMPtr<nsIDOMXPathEvaluator> evaluator;
>+      evaluator = do_CreateInstance(NS_XPATH_EVALUATOR_CONTRACTID);
<..> no if here
>+        gCheckedForXPathDOM = PR_TRUE;
>+        gHaveXPathDOM = (evaluator != nsnull);
<..>
>+      NS_ENSURE_TRUE(evaluator, NS_ERROR_OUT_OF_MEMORY);
as this is a unsupported interface, we should return NS_NOINTERFACE, IMHO

>+      mXPathDocument = new nsXPathDocumentTearoff(evaluator, this);
>+      NS_ENSURE_TRUE(mXPathDocument, NS_ERROR_OUT_OF_MEMORY);
>+    }
>+    foundInterface = mXPathDocument;
>+  }
>+  else

other than that, r=me. tested on solaris debug.

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: v4

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: v4.1

We actually don't need the extra check for !gCheckedForXPathDOM

Comment 20

[Axel Hecht]

Comment on attachment 79262 [details] [diff] [review]
v4.1

r=[e-mail].

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 79262 [details] [diff] [review]
v4.1

+    if (!mXPathDocument) {
+      nsCOMPtr<nsIDOMXPathEvaluator> evaluator;
+      nsresult rv;
+      evaluator = do_CreateInstance(NS_XPATH_EVALUATOR_CONTRACTID, &rv);
+      gCheckedForXPathDOM = PR_TRUE;
+      gHaveXPathDOM = (evaluator != nsnull);
+      if (rv == NS_ERROR_FACTORY_NOT_REGISTERED) {
+	   return NS_ERROR_NO_INTERFACE;
+      }
+      NS_ENSURE_TRUE(evaluator, NS_ERROR_OUT_OF_MEMORY);

Replace the above NS_ENSURE_TRUE(...) with NS_ENSURE_SUCCESS(rv, rv) to
properly propagate all errors to the callers.

sr=jst with that change.

Comment 22

[Peter Van der Beken [:peterv]]

Checked in on the trunk.

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 79262 [details] [diff] [review]
v4.1

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

taking

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in

Comment 26

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.

Comment 27

[Ana]

Hi,

I download the patch. Where am i supposed to install it??? or what am i 
supposed to do with it???
Regards,
Ana