329219 - Box object dangles content reference

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Insert a node into a document
2. Obtain its box object
3. Remove the node and all references to it
4. Attempt to get the node from the box object

nsBoxObject::GetElement segfaults because mContent is stale.

Comment 1

[[e-mail]]

Attachment: Band-aid

I'm assuming here that bug 329181 will clean this up properly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm... we remove the box object from the box object table when a node is removed from the DOM, right?  And never put it back?  Is that true?

If so, this is probably the right fix period.  

Comment 3

[[e-mail]]

Sigh; nsIDOMNSDocument::SetBoxObjectFor is scriptable, which would seem to suggest that content can associate fake box objects with its nodes to confuse extensions... there are only three callers a) GetBoxObjectFor, to store a new box object and b) nsGenericElement/nsXULElement::UnbindFromTree, to forget the box object. But I guess that ought to be handled in a separate bug...

Comment 4

[Daniel Veditz [:dveditz]]

Why do we let web content get a box object, isn't that an internal representation?

Comment 5

[[e-mail]]

(In reply to comment #4)
>Why do we let web content get a box object, isn't that an internal representation?
No, it's what's supposed to exist to protect the internal representation.
(This was the third rewrite, so I gave up and heavily summarised).

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Sigh; nsIDOMNSDocument::SetBoxObjectFor is scriptable

Bug 326931.  I just cced you.  I outline a fix in that bug...

Comment 7

[Daniel Veditz [:dveditz]]

By "mContent is stale" I assume you mean it's a deleted object, and that we therefore want this fix on old branches too?

Comment 8

[[e-mail]]

(In reply to comment #7)
>By "mContent is stale" I assume you mean it's a deleted object
Yes.
>and that we therefore want this fix on old branches too?
As far as I can tell, given my knowledge of security policy.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 213884 [details] [diff] [review]
Band-aid

Fwiw, on trunk I've more or less rolled this into bug 329181.  We should still fix on branch, though.

Comment 10

[[e-mail]]

Fix checked in to the 1.8 branch.

Targetting for 1.8 and marking fixed as per comment 9.

Comment 11

[[e-mail]]

Comment on attachment 213884 [details] [diff] [review]
Band-aid

Trivial stale pointer fix.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 213884 [details] [diff] [review]
Band-aid

approved for 1.8.0 branch, a=dveditz for drivers

Comment 13

[[e-mail]]

Fix checked in to the 1.8.0 branch.

Comment 14

[Jay Patel [:jay]]

Adding [need testcase], can anyone provide a simplified testcase for this bug so we can verify the fix?

If this one-liner is something that can't easily be tested, I think the comments in this bug are enough to marked this v.fixed.  Let us know or just replace "fixed1.8.0.3" with "verified1.8.0.3".  Thanks!

Comment 15

[[e-mail]]

This bookmarklet alerts true on builds with the bug and false on fixed builds.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Reproducing reliably is hard -- it depends on JS GC timing...

Comment 17

[[e-mail]]

I wasn't trying to reproduce the crash, just verify the fix.

Comment 18

[Tracy Walker [:tracy]]

verifying per reporters comment 15