Box object dangles content reference

VERIFIED FIXED

Status

()

Core
XUL
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({verified1.8.0.4, verified1.8.1})

1.8 Branch
verified1.8.0.4, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.14 +
blocking-aviary1.0.9 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][need testcase], URL)

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 213884 [details] [diff] [review]
Band-aid

I'm assuming here that bug 329181 will clean this up properly.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #213884 - Flags: superreview?(bzbarsky)
Attachment #213884 - Flags: review?(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.  
(Assignee)

Comment 3

11 years ago
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...
Why do we let web content get a box object, isn't that an internal representation?
(Assignee)

Comment 5

11 years ago
(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).
> Sigh; nsIDOMNSDocument::SetBoxObjectFor is scriptable

Bug 326931.  I just cced you.  I outline a fix in that bug...
By "mContent is stale" I assume you mean it's a deleted object, and that we therefore want this fix on old branches too?
Flags: blocking1.8.0.3?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Whiteboard: [sg:critical?]
(Assignee)

Comment 8

11 years ago
(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.
Attachment #213884 - Flags: superreview?(bzbarsky)
Attachment #213884 - Flags: superreview+
Attachment #213884 - Flags: review?(bzbarsky)
Attachment #213884 - Flags: review+
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.
Attachment #213884 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 10

11 years ago
Fix checked in to the 1.8 branch.

Targetting for 1.8 and marking fixed as per comment 9.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Version: Trunk → 1.8 Branch
(Assignee)

Comment 11

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

Trivial stale pointer fix.
Attachment #213884 - Flags: approval1.8.0.3?
Attachment #213884 - Flags: approval1.7.14?
Attachment #213884 - Flags: approval-aviary1.0.9?
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3+
Flags: blocking1.7.14?
Flags: blocking1.7.14+
Flags: blocking-aviary1.0.9?
Flags: blocking-aviary1.0.9+
Comment on attachment 213884 [details] [diff] [review]
Band-aid

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #213884 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Assignee)

Comment 13

11 years ago
Fix checked in to the 1.8.0 branch.
Keywords: fixed1.8.0.3

Comment 14

11 years ago
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!
Whiteboard: [sg:critical?] → [sg:critical?][need testcase]
(Assignee)

Comment 15

11 years ago
This bookmarklet alerts true on builds with the bug and false on fixed builds.
Reproducing reliably is hard -- it depends on JS GC timing...
(Assignee)

Comment 17

11 years ago
I wasn't trying to reproduce the crash, just verify the fix.
verifying per reporters comment 15
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.4, fixed1.8.1 → verified1.8.0.4, verified1.8.1
Group: security

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.