319968 - Some DOM functions don't work in XPCShell (classinfo broken)

Status: RESOLVED FIXED

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

Description

[Robert Sayre]

Many DOM functions and fields don't work correctly in XPCShell. For example, array access on a node list, or getElementsByTagName() on a non-document.


js> var domParser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
                      .createInstance(Components.interfaces.nsIDOMParser);
js> var aDom = domParser.parseFromString("<root><feed><entry/><entry/></feed></root>",
                                         "application/xml");
js> var feedList = aDom.getElementsByTagName("feed");
js> feedList
[xpconnect wrapped nsIDOMNodeList]
js> feedList.length
1
js> feedList[0]
js> feedList.item(0)
[xpconnect wrapped nsIDOMNode]
js> var feed = feedList.item(0)
js> feed.localName
feed
js> feed.getElementsByTagName("entry");
typein:30: TypeError: feed.getElementsByTagName is not a function
js>

Comment 1

[timeless]

this is a dom classinfo feature.

nsDOMClassInfo::Init bails out at
  extern nsScriptNameSpaceManager *gNameSpaceManager;
  NS_ENSURE_TRUE(gNameSpaceManager, NS_ERROR_NOT_INITIALIZED);

which means that eager classinfo isn't available. if you manually flatten things, things work. and in my build if i skip that block, everything instantly works.

Comment 2

[timeless]

Attachment: skip bailing code

jst: assuming sayrer confirms that this fixes the problem, would you r+sr and let me land it?

the result is that classinfo is available in xpcshell after you get quite a few complaints about the missing namespace manager. imo this is better than it not working and people complaining about it.

Comment 3

[Robert Sayre]

Attachment: DOM warnings

timeless' patch works as advertised, but causes a crapflood of warnings during parsing (see attached).

Comment 4

[Robert Sayre]

Comment on attachment 207646 [details] [diff] [review]
skip bailing code

let's fix this for real. the console noise is too much.

Comment 5

[Dave Townsend [:mossop]]

Admittedly this is not a major problem for apps but it is now starting to block a number of unit tests I want to write for features. The only alternative would be for me to change half of the component under test to avoid the classinfo stuff.

Comment 6

[Dave Townsend [:mossop]]

Attachment: create NamespaceManager

This code simply initialised gNamespaceManager in nsDOMClassInfo::Init if it isn't already initialised. gNamespaceManager will still get deleted correctly on shutdown in nsJSRuntime::Destroy

Comment 7

[Dave Townsend [:mossop]]

Attachment: add NameSpaceManager getter

Over IRC jst suggested that this was a better approach, creates a getter for the NameSpaceManager that creates it if necessary and make everywhere that uses it call that rather that sharing around the reference.

I'm a little dubious about this assertion I came across: http://mxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#5033

Seems to assert without the manager but then continue to call the namespacemanager which would crash, an NS_ENSURE_SUCCESS would seem better to me. I didn't change for now though maybe it's impossible to follow those paths without it.

Comment 8

[Dave Townsend [:mossop]]

This is from xpcshell with the patch:

s> var domParser = Components.classes["@mozilla.org/xmlextras/domparser;1"].createInstance(Components.interfaces.nsIDOMParser);
js> var aDom = domParser.parseFromString("<root><feed><entry/><entry/></feed></root>","application/xml");
js> var feedList = aDom.getElementsByTagName("feed");
js> feedList
[object HTMLCollection @ 0x11011630 (native @ 0x1101a420)]
js> feedList.length
1
js> feedList[0]
[object Element @ 0x1101b2c0 (native @ 0x11019a30)]
js> feedList[0].localName
feed
js> feedList[0].getElementsByTagName("entry")
[object HTMLCollection @ 0x1101c210 (native @ 0x1101c170)]

Comment 9

[Dave Townsend [:mossop]]

Attachment: same patch with unit test

This is the same patch but with an added unit test checking that classinfo usage works.

One thing I forgot to mention, this patch does not totally bring classinfo to xpcshell. The main xpcshell js context doesn't get the automatic DOM globals (Element, Node etc.) so for example you can't test |entry instanceof Element|. I'm pretty sure that making that work would require making xpcshell depend on dom which I've been told is not wanted.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 285480 [details] [diff] [review]
same patch with unit test

+//static
+nsresult
+nsJSRuntime::GetNameSpaceManager(nsScriptNameSpaceManager **aNameSpaceManager)

It doesn't seem like any callers care what the error code is, could we simply return the nsScriptNameSpaceManager and eliminate the nsresult all together? Less code, unless there's a reason to keep the error code around...

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> It doesn't seem like any callers care what the error code is, could we simply
> return the nsScriptNameSpaceManager and eliminate the nsresult all together?
> Less code, unless there's a reason to keep the error code around...

I guess not no, will make that change tomorrow.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #10)
> > It doesn't seem like any callers care what the error code is, could we simply
> > return the nsScriptNameSpaceManager and eliminate the nsresult all together?
> > Less code, unless there's a reason to keep the error code around...
> 
> I guess not no, will make that change tomorrow.

See http://wiki.mozilla.org/Gecko:Interface_Style_Guide where it is written:

"A getter method that returns a single bare pointer which can never be null should lose the "Get" prefix. Thus no-parameter methods whose names are noun phrases indicate that it is safe to use their result without testing for null."

DeCOMtamination is good, just remember to rename to a noun phrase if the getter becomes infallible.

/be

Comment 13

[Dave Townsend [:mossop]]

Attachment: Simplify getter

This simplifies the getter as requested. It is still possible for it to return null so kept the name the same.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 285878 [details] [diff] [review]
Simplify getter

Looks good! r+sr=jst

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 285878 [details] [diff] [review]
Simplify getter

This patch ensures that js operations involving classinfo occuring in components behave the same in an unit test environment as they do in the full browser.

The fix is to always create the namespace manager whenever it is needed. In the browser this was always created early so I would not expect any change in behaviour for the actual app

Comment 16

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

Not a blocker, but probably appropriate for approval after beta branches.

Comment 17

[Reed Loden [:reed]]

Checking in dom/src/base/nsDOMClassInfo.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v  <--  nsDOMClassInfo.cpp
new revision: 1.491; previous revision: 1.490
done
Checking in dom/src/base/nsDOMScriptObjectFactory.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMScriptObjectFactory.cpp,v  <--  nsDOMScriptObjectFactory.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in dom/src/base/nsJSEnvironment.cpp;
/cvsroot/mozilla/dom/src/base/nsJSEnvironment.cpp,v  <--  nsJSEnvironment.cpp
new revision: 1.366; previous revision: 1.365
done
Checking in dom/src/base/nsJSEnvironment.h;
/cvsroot/mozilla/dom/src/base/nsJSEnvironment.h,v  <--  nsJSEnvironment.h
new revision: 1.95; previous revision: 1.94
done
Checking in dom/tests/Makefile.in;
/cvsroot/mozilla/dom/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/unit/test_bug319968.js,v
done
Checking in dom/tests/unit/test_bug319968.js;
/cvsroot/mozilla/dom/tests/unit/test_bug319968.js,v  <--  test_bug319968.js
initial revision: 1.1
done