Closed
Bug 329335
Opened 19 years ago
Closed 19 years ago
Crash [@ nsXULDocument::~nsXULDocument] involving <svg:svg datasources=""> in XUL
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: enndeakin)
References
(Depends on 1 open bug)
Details
(Keywords: crash, regression, testcase, Whiteboard: [sg:critical] after 1.8)
Crash Data
Attachments
(3 files, 1 obsolete file)
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
Mac trunk (both release and debug) crashes if you load the testcase and wait a few seconds. I think the crash is exploitable.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Reporter | ||
Updated•19 years ago
|
Whiteboard: [sg:critical]
Reporter | ||
Comment 3•19 years ago
|
||
The testcase reloads itself over and over until it crashes, so it crashes faster (e.g. in 2 seconds instead of 5) if you save it and then load your local copy.
Comment 4•19 years ago
|
||
I confirm the crash using windows on the trunk, calling a method on a deleted observer vtbl. The 1.8.0 branch does not crash. Neither does my 1.8 branch build, but it's a month old so we should check whether the current ff2 does: might give us a regression range or point us at trunk-only changes.
Keywords: regression
Comment 5•19 years ago
|
||
This regressed between 2006-02-13-06 and 2006-02-14-08. Given "datasources", I posit bug 285631 is responsible.
Updated•19 years ago
|
Flags: blocking1.9a1+
Updated•19 years ago
|
Assignee: nobody → enndeakin
Assignee | ||
Comment 6•19 years ago
|
||
My stack trace isn't identical but similar. The patch should also fix bugs 323988 and 327508.
Attachment #214219 -
Flags: superreview?(bzbarsky)
Attachment #214219 -
Flags: review?(bzbarsky)
Comment 7•19 years ago
|
||
We're a document observer, no? Why can't we just handle this in ContentRemoved without having to add support elsewhere? Fwiw, I bet the null-check in nsXULDocument::SetTemplateBuilderFor was supposed to be checking aBuilder, not aContent...
Assignee | ||
Comment 8•19 years ago
|
||
(In reply to comment #7) > We're a document observer, no? Why can't we just handle this in ContentRemoved > without having to add support elsewhere? I originally did implement it that way, but ContentRemoved didn't seem to be called recursively. > > Fwiw, I bet the null-check in nsXULDocument::SetTemplateBuilderFor was supposed > to be checking aBuilder, not aContent... > Likely, although there are no callers that pass null. I can change it if desired.
Status: NEW → ASSIGNED
Comment 9•19 years ago
|
||
> but ContentRemoved didn't seem to be called recursively. It's not -- you need to check whether the content removed is an ancestor of the root. > Likely, although there are no callers that pass null. Ah. Then your change is OK if an assert is added that null is not being passed, I guess.
Reporter | ||
Updated•19 years ago
|
Assignee | ||
Comment 10•19 years ago
|
||
Attachment #214219 -
Attachment is obsolete: true
Attachment #214314 -
Flags: superreview?(bzbarsky)
Attachment #214314 -
Flags: review?(bzbarsky)
Attachment #214219 -
Flags: superreview?(bzbarsky)
Attachment #214219 -
Flags: review?(bzbarsky)
Comment 11•19 years ago
|
||
Comment on attachment 214314 [details] [diff] [review] Use ContentRemoved instead for notifications >Index: content/xul/templates/src/nsXULTemplateBuilder.cpp >+nsXULTemplateBuilder::ContentRemoved(nsIDocument* aDocument, >+ nsIContent* parent = mRoot; >+ >+ while (parent) { >+ if (parent == aChild) { How about: if (nsContentUtils::ContentIsDescendantOf(mRoot, aChild)) { // Do cleanup here instead of doing the while loop yourself? r+sr=bzbarsky with that change.
Attachment #214314 -
Flags: superreview?(bzbarsky)
Attachment #214314 -
Flags: superreview+
Attachment #214314 -
Flags: review?(bzbarsky)
Attachment #214314 -
Flags: review+
Assignee | ||
Comment 12•19 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 13•19 years ago
|
||
This apparently caused bug 330167. ;(
Updated•18 years ago
|
Whiteboard: [sg:critical] → [sg:critical] after 1.8
Comment 14•18 years ago
|
||
Just wondering, could this have fixed bug 326468?
Updated•18 years ago
|
Group: security
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Updated•13 years ago
|
Crash Signature: [@ nsXULDocument::~nsXULDocument]
You need to log in
before you can comment on or make changes to this bug.
Description
•