46964 - crash calling rebuild() twice

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Chris Waterson]

To reproduce:

1. Save the attached test case as "test.xul"
2. Open it in a browser window.
3. Click the button
4. Click the button again. *boom*

Expected: no crash

Comment 1

[Chris Waterson]

Attachment: test case

Comment 2

[Chris Waterson]

The problem is that the ContentSupportMap entries hold a pointer into the
ConflictSet's arena. (Ok, I feel like I'm talking about the pyramids of Mars
layout engine right now. I'll just shut up and attach a fix.)

Comment 3

[Chris Waterson]

Attachment: clear content support map before clearing conflict set

Comment 4

[Chris Waterson]

Ok, so some explanation is in order.

The "conflict set" is the data structure that keeps track of which of the
template rules are currently matched. Since a template rule depends on content
in the content model (e.g., if you remove a "parent" node from the tree, all the
rules that matched for that nodes children clearly don't match anymore).

The "content support map" keeps track of which rules are supported by elements
in the content model. It maps a content node to a "match".

Problem is, the conflict set owns all of the match objects. Calling
mConflictSet.Clear() in nsXULTemplateBuilder::Rebuild() causes the arena pool
that was containing all the match objects to be dumped en masse and re-created.
This leaves the content support map to be left with a dangling pointer into the
conflict set. The above test case causes us to actually try to use one of those
pointers.

The fix is to nuke the content support map before we nuke the conflict set.

Comment 5

[Blake Ross]

spam: Adding crash keyword...

Comment 6

[Chris Waterson]

I don't like my first fix, and spent last night working up the one I'm going to 
attach shortly. I don't like it either, for reason's I'll explain 
momentarily...

Comment 7

[Chris Waterson]

Attachment: remove ContentSupportMap

Comment 8

[Chris Waterson]

The attached patch (besides commenting a bunch of junk) removes the 
ContentSupportMap structure. This structure was used to implement 
CreateTemplateContents(): it recorded the match from whence each piece of 
generated content was created so that we could quickly pick up and resume 
template building.

I thought that removing it would be a good thing to do, unfortunately, it slows 
folder opening down by about 7%. I thought I'd shove it into the bug for 
posterity's sake (or maybe to deal with later).

Comment 9

[Chris Waterson]

Attachment: like the first patch, but better.

Comment 10

[Chris Waterson]

fix checked in