Closed Bug 445177 Opened 14 years ago Closed 14 years ago

Crash [@ nsContentUtils::ComparePosition(nsINode*, nsINode*) ] with multipe identic ids

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: arno, Assigned: smaug)

References

Details

(Keywords: fixed1.9.1, regression, verified1.9.0.12, Whiteboard: [sg:dos] null deref, but maybe scary during mutation?)

Attachments

(4 files, 3 obsolete files)

Attached file testcase
splitted from bug #442227 comment #8
From what I understand, (at least part of) problem lies in id attributtes handling. When document is parsed, <code/> will observe for first <box> as it is document.getElementById("1") at that time. Then, when <code> is removed from document, RemoveBroadcastListenerFor will be called with second box as an argument, as it's document.getElementById("1") at that time. I don't known how to solve that problem.
Maybe related to bug 395671?
I think we may want to try to sync broadcasters asyncronously, or in EndUpdate.
Assignee: jag → Olli.Pettay
Attachment #337730 - Flags: superreview?(neil)
Attachment #337730 - Flags: review?(enndeakin)
Comment on attachment 337730 [details] [diff] [review]
delay until it is ok to synchronize

I don't know what EndUpdate does, maybe you should ask someone who does ;-)
Attachment #337730 - Flags: superreview?(neil)
Attachment #337730 - Flags: superreview?(bzbarsky)
So why does this patch fix the bug?  Comment 1 doesn't sound like it would be fixed by the patch...
There are mutation events while we're in document update - things aren't in very
consistent state.
But true, I need to take care of ID attrs.
Attachment #337730 - Flags: superreview?(bzbarsky)
Attachment #337730 - Flags: review?(enndeakin)
Actually, in this particular testcase the patch should work just fine. But there are other possible problems which we should fix too.
The first patch (included in this one) is needed to fix the crash, but to 
properly cut listener->broadcaster link when removing listener from document
we need to go through all the broadcasters - scripts may have used .addBroadcastListenerFor().
Currently we don't remove broadcasters from the map, but that should be ok,
since broadcasters are used just as a key in the hashmap.
(I think we could actually remove BroadcasterMapEntry::mBroadcaster)
Attachment #337730 - Attachment is obsolete: true
Attachment #337899 - Flags: review?(enndeakin)
Comment on attachment 337899 [details] [diff] [review]
+remove listener properly from map

Seems OK. Can the testcase be incorporated?
Attachment #337899 - Flags: review?(enndeakin) → review+
Yes, I need to write some tests. And there is also the crashtest - but that can be checked in only after porting the fix to branches. (Although it is a null pointer crash, I think using mutation listeners one can do pretty evil things.)
Comment on attachment 337899 [details] [diff] [review]
+remove listener properly from map

Boris, what do you think about this version?
Attachment #337899 - Flags: superreview?(bzbarsky)
The change to current behavior are tests 10 and 11
Still, if we want to be very conservative, the first patch is enough.
It would be great if someone defined broadcaster-listener behavior in the case
of dynamic changes.
Attachment #337730 - Attachment is obsolete: false
Attachment #337730 - Flags: superreview?(bzbarsky)
Attachment #337730 - Flags: review?(bzbarsky)
Comment on attachment 337730 [details] [diff] [review]
delay until it is ok to synchronize

This isn't obsolete after all. This is the crashfix! The other patch changes the behavior a bit more. So for 1.9.0.x this is better. And the more I think about this all, the more I like this one.
Comment on attachment 337730 [details] [diff] [review]
delay until it is ok to synchronize

Add a comment about why synchronizing in mid-update is bad, perhaps?
Attachment #337730 - Flags: superreview?(bzbarsky)
Attachment #337730 - Flags: superreview+
Attachment #337730 - Flags: review?(bzbarsky)
Attachment #337730 - Flags: review+
Comment on attachment 337899 [details] [diff] [review]
+remove listener properly from map

sr=bzbarsky, but watch out for performance regressions here...
Attachment #337899 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 337730 [details] [diff] [review]
delay until it is ok to synchronize

I pushed this patch. Should get some backing time before landing to 1.9.0. And fixing the handling of dynamic changes isn't security sensitive.

author	Olli Pettay <Olli.Pettay@helsinki.fi>
	Tue Sep 16 11:53:30 2008 +0300 (at Tue Sep 16 11:53:30 2008 +0300)
changeset 19296	30306c8bfa2f
parent 19295	519540dd7880
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This particular testcase causes a null deref, but I'm with Olli in comment 11 being suspicious of crashes due to mutation events. I assume seven or eight  months (!) is enough baking time?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.11?
Flags: blocking1.9.0.11? → wanted1.9.1?
Whiteboard: [sg:dos] null deref, but maybe scary during mutation?
Blocks: 391002
No longer depends on: 391002
Flags: wanted1.9.1?
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12+
Keywords: regression
Attached patch for 1.9.0 (obsolete) — Splinter Review
This should fix bug 472668, bug 468211,  bug 445177.

What I'm not quite sure is about the nsINode flag. Is that ok for 1.9.0?
Attachment #382278 - Flags: superreview?(bzbarsky)
Attachment #382278 - Flags: review?(bzbarsky)
> +        if (length) {

That can't possibly be 0, right?

For the flag... can we set it to 0x80000000?  That should be pretty much guaranteed to be unused at the moment.  The change as written will make code compiled against the old header see this flag when it expects the script type, I think.
Attached patch v2 for 190 (obsolete) — Splinter Review
Attachment #382278 - Attachment is obsolete: true
Attachment #382578 - Flags: superreview?(bzbarsky)
Attachment #382578 - Flags: review?(bzbarsky)
Attachment #382278 - Flags: superreview?(bzbarsky)
Attachment #382278 - Flags: review?(bzbarsky)
Attachment #382578 - Flags: superreview?(bzbarsky)
Attachment #382578 - Flags: superreview+
Attachment #382578 - Flags: review?(bzbarsky)
Attachment #382578 - Flags: review+
Attachment #382578 - Flags: approval1.9.0.12?
Attachment #382578 - Flags: approval1.9.0.12? → approval1.9.0.12+
Comment on attachment 382578 [details] [diff] [review]
v2 for 190

Approved for 1.9.0.12, a=dveditz for release-drivers
Checking in content/base/public/nsINode.h;
/cvsroot/mozilla/content/base/public/nsINode.h,v  <--  nsINode.h
new revision: 3.78; previous revision: 3.77
done
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.833; previous revision: 1.832
done
Checking in content/xul/document/src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.210; previous revision: 1.209
done
Keywords: fixed1.9.0.12
Backed out because of leak orange.
Keywords: fixed1.9.0.12
Argh, trunk has the "delay until it is ok to synchronize",
not "+remove listener properly from map".
New patch for 190 coming.
I should have read comment 16.
Attachment #337899 - Attachment is obsolete: true
No need for the nsINode flag.
Attachment #382578 - Attachment is obsolete: true
Attachment #383180 - Flags: superreview?(bzbarsky)
Attachment #383180 - Flags: review?(bzbarsky)
Attachment #383180 - Flags: approval1.9.0.12?
Attachment #383180 - Flags: superreview?(bzbarsky)
Attachment #383180 - Flags: superreview+
Attachment #383180 - Flags: review?(bzbarsky)
Attachment #383180 - Flags: review+
Comment on attachment 383180 [details] [diff] [review]
strict subset of the previous patch

I sort of trust you that this fixes the crash bug; I'm lost in all the patch variants here.
The patch is basically the same that landed on trunk + script runner changes also landed on trunk, like https://bugzilla.mozilla.org/attachment.cgi?id=358815&action=edit
Attachment #382578 - Flags: approval1.9.0.12+
Attachment #383180 - Flags: approval1.9.0.12? → approval1.9.0.12+
Comment on attachment 383180 [details] [diff] [review]
strict subset of the previous patch

Approved for 1.9.0.12. a=ss
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.835; previous revision: 1.834
done
Checking in content/xul/document/src/nsXULDocument.h;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.h,v  <--  nsXULDocument.h
new revision: 1.212; previous revision: 1.211
done
Keywords: fixed1.9.0.12
Verified fixed in 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729) using attached testcase. 1.9.0.11 crashes on same system.
Does not affect 1.8.
Flags: wanted1.8.1.x-
Group: core-security
You need to log in before you can comment on or make changes to this bug.