223799 - [FIXr]nsXBLContentSink::ConstructField mImplementation == null with bad XBL

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Alex Vincent [:WeirdAl]]

For some reason, mImplementation == null, and we're not checking for that first.

The null value is probably a result of bad XBL markup, which I will attach as a
testcase.  The second attachment will be a stack trace.

Thanks to dwitte for assisting on the basics of gdb and on 0x0 being a null
value.  (I'm just learning C++.)

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: crash testcase

Expected results:  Page loads, throws an exception for bad XBL markup.
Actual results:  Crash.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: stack trace

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm tired of this ridiculous whack-a-mole with the XBL sink's crashes.  The
basic problem is that this code, as written, assumes that all XBL documents are
perfectly well-formed.  That's got to stop.  We need a clear list of what is
allowed/supposed to be contained in what in XBL, and then that list needs to be
turned into code in a systematic manner (eg. "ignore all these things if not in
a binding") instead of fixing testcases ad-hoc as people write XBL that crashes us.

ccing some people who may be able to provide the containment information needed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

When I say "well-formed", I don't mean in an XML sense -- expat ensures that,
more or less.  I mean in the sense of what an XBL document is "supposed" to look
like.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and we should decide what should happen to markup like this.  Should we
silently ignore the stuff that does not belong?  Or should we call ReportError
as needed (and maybe cancel the rest of the parse)?

Comment 6

[Alex Vincent [:WeirdAl]]

I'd offer to write a doctype for XBL, except Mozilla doesn't validate by doctypes...

Comment 7

[Benjamin Smedberg]

IMO we should fail entirely if we encounter unexpected markup. Partial XBL
bindings are worse than none at all ;)

Comment 8

[Alex Vincent [:WeirdAl]]

Eh, a doctype wouldn't help anyway, as my XUL docs almost never have doctypes.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ben, see nsXBLContentSink::ReportError... ;)  There is no real way to cleanly
tear down a partial binding thus far.

What I suggest is that for now we come up with a list of what's OK, then make
the sink stop processing once it hits the first "not OK" thing (just set an
error boolean).  Then work on a way to drop the binding too.

WeirdAl, I'm talking a human-readable (read: Boris-readable) document here, not
a DTD... ;)

Comment 10

[Hixie (not reading bugmail)]

bz: I can provide you with such a document.

Comment 11

[[e-mail]]

Hopefully http://www.mozdev.org/source/browse/xblref/docs/ is still valid...

Comment 12

[Hixie (not reading bugmail)]

Note that supporting what bz suggests musn't prevent XBL from being included 
inline in another XML file and addressed by a fragment identifier.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, looks like http://www.w3.org/TR/xbl/ in fact contains the desired
information, assuming it's up-to-date enough.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Looks like that spec knows nothing about <resources>....

Comment 15

[Benjamin Smedberg]

bz: that is a very old spec, doesn't do <constructor> and other stuff: you need
to get a new spec from hixie/hyatt

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Near as I can make it...

This fixes the crash, and even reports the error. I wish we had a simpler way
to report errors... :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yeah, well.  That spec from hixie was promised over a month ago and never
materialized.  Crashes are bad.  ergo...

The patch is based on the existing code, the docs neil pointed to, and the w3
spec.  <shrug>.

Comment 18

[timeless]

Comment on attachment 136769 [details] [diff] [review]
Near as I can make it...

w/ some minor changes discussed on irc.
w/ or w/o the one optional change discussed on irc.

Comment 19

[Hixie (not reading bugmail)]

Sorry, there have been issues. You should have caught me on IRC, I would have 
given you an unofficial URI.

Anyway, here's the spec: http://www.hixie.ch/specs/xbl/xbl

I didn't check if it addressed the issue this bug was talking about. Let me know 
if it doesn't and I'll fix it. (Let me know how you would like me to fix it, 
too, as I don't have any strong opinions on the matter.)

Comment 20

[Boris Zbarsky [:bzbarsky]]

It sorta does, yes...  I don't believe our current impl supports <script> (I
can't find anything that does so, at least), and according to that spec,
<constructor> and <destructor> are not allowed in <implementation> (not allowed
anywhere, in fact).  That should probably be fixed.

Other than that, that spec matches up with my code (modulo the fact that the XBL
content sink does nothing special for <children>, hence we would probably allow
it in all sorts of weird places).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Taking, I guess.

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 136769 [details] [diff] [review]
Near as I can make it...

>Index: content/xbl/src/nsXBLContentSink.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLContentSink.cpp,v
>retrieving revision 1.34
>diff -p -u -1 -0 -r1.34 nsXBLContentSink.cpp
>--- content/xbl/src/nsXBLContentSink.cpp	17 Nov 2003 21:03:30 -0000	1.34
>+++ content/xbl/src/nsXBLContentSink.cpp	4 Dec 2003 03:46:07 -0000
>-      if (mState == eXBL_InImplementation && tagAtom == nsXBLAtoms::implementation)
>-        mState = eXBL_InBinding;
>-      else if (mState == eXBL_InBinding && tagAtom == nsXBLAtoms::binding) {
>-        mState = eXBL_InDocument;
>+      if (mState == eXBL_InBinding && tagAtom == nsXBLAtoms::binding) {
>+        mState = eXBL_InBindings;
>         mBinding->Initialize();
>         mBinding = nsnull; // Clear our current binding ref.

Any idea why the InBinding -> InBindings transition is after the call to
nsXMLContentSink::HandleEndElement, but all the others are before it? (I hope
I'm correct that the InImplementation->InBinding transition here is being
removed because it's redundant).

sr=me.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yeah, that's redundant code.  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xbl/src/nsXBLContentSink.cpp&rev=1.34&mark=279-281,310-311#279
(which marks the code that actually runs as well as the code I'm removing).

As for the ordering thing, it's been that way since initial checkin (rev 1.1). 
I _think_ it's because there should be no nodes for those other elements, so the
close calls need to return before calling the parent.  The transition could
still be done first, but this clearly differentiates the two types of elements
(or would if a comment got added).

I'll probably be filing a follow-up bug to investigate that in more detail,
since some logic there is wacky and triggers asserts in namespace code in the
XML content sink (when I add asserts that really should be there).

Comment 24

[Bob Clary [:bc] (inactive)]

You may be interested in a relaxNG grammar for XBL I have started based
initially on Hixie's XBL spec. See <http://dev.bclary.com/xbl-relaxNG/xbl.rng>
and <http://dev.bclary.com/xbl-relaxNG/xbl.rnc>. The compact version is very
useful with Emacs and Jame Clark's nxml-mode major mode. Comments welcome.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I filed bug 230236 on the followup issues described in comment 23.

Checked in to 1.7a.

Comment 26

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1