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

RESOLVED FIXED in mozilla1.7alpha

Status

()

P1
critical
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: WeirdAl, Assigned: bzbarsky)

Tracking

({crash, testcase})

Trunk
mozilla1.7alpha
x86
Linux
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

15 years ago
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++.)
(Reporter)

Comment 1

15 years ago
Created attachment 134225 [details]
crash testcase

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

Comment 2

15 years ago
Created attachment 134226 [details]
stack trace
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.
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.
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)?
(Reporter)

Comment 6

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

Comment 7

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

Comment 8

15 years ago
Eh, a doctype wouldn't help anyway, as my XUL docs almost never have doctypes.
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... ;)
bz: I can provide you with such a document.
Note that supporting what bz suggests musn't prevent XBL from being included 
inline in another XML file and addressed by a fragment identifier.
OK, looks like http://www.w3.org/TR/xbl/ in fact contains the desired
information, assuming it's up-to-date enough.
Looks like that spec knows nothing about <resources>....

Comment 15

15 years ago
bz: that is a very old spec, doesn't do <constructor> and other stuff: you need
to get a new spec from hixie/hyatt
Created attachment 136769 [details] [diff] [review]
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... :(
(Assignee)

Updated

15 years ago
Attachment #136769 - Flags: review?(timeless)
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

15 years ago
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.
Attachment #136769 - Flags: review?(timeless) → review+
(Assignee)

Updated

15 years ago
Attachment #136769 - Flags: superreview?(bryner)
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.)
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).
Taking, I guess.
Assignee: hyatt → bz-vacation
Priority: -- → P1
Summary: nsXBLContentSink::ConstructField mImplementation == null with bad XBL → [FIX]nsXBLContentSink::ConstructField mImplementation == null with bad XBL
Target Milestone: --- → mozilla1.7alpha
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.
Attachment #136769 - Flags: superreview?(bryner) → superreview+
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).
Summary: [FIX]nsXBLContentSink::ConstructField mImplementation == null with bad XBL → [FIXr]nsXBLContentSink::ConstructField mImplementation == null with bad XBL

Comment 24

15 years ago
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.
I filed bug 230236 on the followup issues described in comment 23.

Checked in to 1.7a.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 26

10 years ago
Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.