Last Comment Bug 329219 - Box object dangles content reference
: Box object dangles content reference
Status: VERIFIED FIXED
[sg:critical?][need testcase]
: verified1.8.0.4, verified1.8.1
Product: Core
Classification: Components
Component: XUL (show other bugs)
: 1.8 Branch
: All All
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
: Neil Deakin
Mentors:
javascript: var foo = new Image; docu...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-03 03:20 PST by neil@parkwaycc.co.uk
Modified: 2008-07-31 03:20 PDT (History)
6 users (show)
dveditz: blocking1.7.14+
dveditz: blocking‑aviary1.0.9+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Band-aid (654 bytes, patch)
2006-03-03 03:29 PST, neil@parkwaycc.co.uk
bzbarsky: review+
bzbarsky: superreview+
neil: approval‑aviary1.0.9?
neil: approval1.7.14?
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2006-03-03 03:20:06 PST
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 neil@parkwaycc.co.uk 2006-03-03 03:29:28 PST
Created attachment 213884 [details] [diff] [review]
Band-aid

I'm assuming here that bug 329181 will clean this up properly.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2006-03-03 07:28:32 PST
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 neil@parkwaycc.co.uk 2006-03-03 09:19:31 PST
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] 2006-03-03 09:26:45 PST
Why do we let web content get a box object, isn't that an internal representation?
Comment 5 neil@parkwaycc.co.uk 2006-03-03 09:45:48 PST
(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 [:bz] (still a bit busy) 2006-03-03 10:43:32 PST
> Sigh; nsIDOMNSDocument::SetBoxObjectFor is scriptable

Bug 326931.  I just cced you.  I outline a fix in that bug...
Comment 7 Daniel Veditz [:dveditz] 2006-03-03 12:47:29 PST
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 neil@parkwaycc.co.uk 2006-03-03 13:04:45 PST
(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 [:bz] (still a bit busy) 2006-03-05 21:09:21 PST
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 neil@parkwaycc.co.uk 2006-03-06 02:12:54 PST
Fix checked in to the 1.8 branch.

Targetting for 1.8 and marking fixed as per comment 9.
Comment 11 neil@parkwaycc.co.uk 2006-03-06 02:42:17 PST
Comment on attachment 213884 [details] [diff] [review]
Band-aid

Trivial stale pointer fix.
Comment 12 Daniel Veditz [:dveditz] 2006-04-03 12:13:47 PDT
Comment on attachment 213884 [details] [diff] [review]
Band-aid

approved for 1.8.0 branch, a=dveditz for drivers
Comment 13 neil@parkwaycc.co.uk 2006-04-03 15:47:26 PDT
Fix checked in to the 1.8.0 branch.
Comment 14 Jay Patel [:jay] 2006-04-20 15:20:53 PDT
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 neil@parkwaycc.co.uk 2006-04-20 16:04:45 PDT
This bookmarklet alerts true on builds with the bug and false on fixed builds.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2006-04-20 17:09:09 PDT
Reproducing reliably is hard -- it depends on JS GC timing...
Comment 17 neil@parkwaycc.co.uk 2006-04-21 01:56:51 PDT
I wasn't trying to reproduce the crash, just verify the fix.
Comment 18 Tracy Walker [:tracy] 2006-05-05 11:36:13 PDT
verifying per reporters comment 15

Note You need to log in before you can comment on or make changes to this bug.