233641 - [FIX]out-of-bounds inserts in nsXULElement::InsertChildAt on startup

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

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

Just starting the browser results in calls to nsXULElement::InsertChildAt with
indexes bigger then number of children in a node. This should not happen as such
inserts will fail (note that the index is even bigger then for an append).
Seemed to be some problem in the overlay code. Possibly an off-by-one error.

Comment 1

[Brendan Eich [:brendan]]

Is this when deserializing a FastLoad file?

/be

Comment 2

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

No, we deserialize into prototype nodes, which this code doesn't deal with.

It seemed to be during the process of creating and wrapping a DOM-tree around a
prototype-tree. Though of course it could be more then that. I didn't look into
why this was called once i realized i had to deal with it still.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

The problem here is that overlays can specify an explicit insertion index (via
an attribute).	This code needs to sanity-check the number before blindly
trying to insert at the provided index.

(Note that I suspect that overlaying non-XUL elements like this still asserts
even in today's builds, so we really do want to fix this underlying bug...)

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'd really like to get this in for 1.8...

Comment 5

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

> (Note that I suspect that overlaying non-XUL elements like this still asserts
> even in today's builds, so we really do want to fix this underlying bug...)

I don't get this comment. I don't see anything xul specific about your fix...

Comment 6

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

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

r=me given a clarification on the question in previous comment.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I don't get this comment. I don't see anything xul specific about your fix...

I'm removing code in nsXULElement that protected against this bug in overlay
merging.  nsGenericElement doesn't have equivalent code, so in a build without
my fix, if we overlay kids into a non-XUL node in a XUL document we'll end up
first asserting and then referencing out-of-bounds elements in the child array
of the nsGenericElement.  That's all I was saying.

Comment 8

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

Ah, i'm with you.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

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

sr=[e-mail]

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed.