Last Comment Bug 445177 - Crash [@ nsContentUtils::ComparePosition(nsINode*, nsINode*) ] with multipe identic ids
: Crash [@ nsContentUtils::ComparePosition(nsINode*, nsINode*) ] with multipe i...
Status: RESOLVED FIXED
[sg:dos] null deref, but maybe scary ...
: fixed1.9.1, regression, verified1.9.0.12
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Neil Deakin
Mentors:
Depends on:
Blocks: 391002
  Show dependency treegraph
 
Reported: 2008-07-14 10:17 PDT by arno renevier
Modified: 2009-07-21 21:25 PDT (History)
10 users (show)
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
stransky: wanted1.8.1.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (864 bytes, application/vnd.mozilla.xul+xml)
2008-07-14 10:17 PDT, arno renevier
no flags Details
delay until it is ok to synchronize (3.98 KB, patch)
2008-09-09 13:47 PDT, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
+remove listener properly from map (7.47 KB, patch)
2008-09-10 10:15 PDT, Olli Pettay [:smaug]
enndeakin: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
some mochitests (3.97 KB, patch)
2008-09-11 08:52 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
for 1.9.0 (8.62 KB, patch)
2009-06-09 05:02 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v2 for 190 (8.60 KB, patch)
2009-06-10 13:52 PDT, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
strict subset of the previous patch (4.88 KB, patch)
2009-06-14 09:55 PDT, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
samuel.sidler+old: approval1.9.0.12+
Details | Diff | Splinter Review

Description arno renevier 2008-07-14 10:17:10 PDT
Created attachment 329484 [details]
testcase

splitted from bug #442227 comment #8
Comment 1 arno renevier 2008-07-14 10:57:08 PDT
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.
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-07-14 11:09:47 PDT
Maybe related to bug 395671?
Comment 3 Olli Pettay [:smaug] 2008-09-09 03:42:48 PDT
I think we may want to try to sync broadcasters asyncronously, or in EndUpdate.
Comment 4 Olli Pettay [:smaug] 2008-09-09 13:47:58 PDT
Created attachment 337730 [details] [diff] [review]
delay until it is ok to synchronize
Comment 5 neil@parkwaycc.co.uk 2008-09-10 03:38:08 PDT
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 ;-)
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2008-09-10 07:13:12 PDT
So why does this patch fix the bug?  Comment 1 doesn't sound like it would be fixed by the patch...
Comment 7 Olli Pettay [:smaug] 2008-09-10 07:39:24 PDT
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.
Comment 8 Olli Pettay [:smaug] 2008-09-10 07:50:11 PDT
Actually, in this particular testcase the patch should work just fine. But there are other possible problems which we should fix too.
Comment 9 Olli Pettay [:smaug] 2008-09-10 10:15:57 PDT
Created attachment 337899 [details] [diff] [review]
+remove listener properly from map

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)
Comment 10 Neil Deakin 2008-09-10 11:56:12 PDT
Comment on attachment 337899 [details] [diff] [review]
+remove listener properly from map

Seems OK. Can the testcase be incorporated?
Comment 11 Olli Pettay [:smaug] 2008-09-10 13:24:50 PDT
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 12 Olli Pettay [:smaug] 2008-09-10 13:25:15 PDT
Comment on attachment 337899 [details] [diff] [review]
+remove listener properly from map

Boris, what do you think about this version?
Comment 13 Olli Pettay [:smaug] 2008-09-11 08:52:08 PDT
Created attachment 338120 [details] [diff] [review]
some mochitests
Comment 14 Olli Pettay [:smaug] 2008-09-11 08:54:45 PDT
The change to current behavior are tests 10 and 11
Comment 15 Olli Pettay [:smaug] 2008-09-11 09:17:06 PDT
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.
Comment 16 Olli Pettay [:smaug] 2008-09-11 09:21:54 PDT
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 17 Boris Zbarsky [:bz] (still a bit busy) 2008-09-15 13:35:31 PDT
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?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2008-09-15 13:37:45 PDT
Comment on attachment 337899 [details] [diff] [review]
+remove listener properly from map

sr=bzbarsky, but watch out for performance regressions here...
Comment 19 Olli Pettay [:smaug] 2008-09-16 01:56:39 PDT
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
Comment 20 Daniel Veditz [:dveditz] 2009-05-04 23:03:39 PDT
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?
Comment 21 Olli Pettay [:smaug] 2009-06-09 05:02:23 PDT
Created attachment 382278 [details] [diff] [review]
for 1.9.0

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?
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2009-06-09 07:59:14 PDT
> +        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.
Comment 23 Olli Pettay [:smaug] 2009-06-10 13:52:32 PDT
Created attachment 382578 [details] [diff] [review]
v2 for 190
Comment 24 Daniel Veditz [:dveditz] 2009-06-12 10:21:07 PDT
Comment on attachment 382578 [details] [diff] [review]
v2 for 190

Approved for 1.9.0.12, a=dveditz for release-drivers
Comment 25 Olli Pettay [:smaug] 2009-06-14 08:13:13 PDT
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
Comment 26 Olli Pettay [:smaug] 2009-06-14 08:56:10 PDT
Backed out because of leak orange.
Comment 27 Olli Pettay [:smaug] 2009-06-14 09:19:30 PDT
Argh, trunk has the "delay until it is ok to synchronize",
not "+remove listener properly from map".
New patch for 190 coming.
Comment 28 Olli Pettay [:smaug] 2009-06-14 09:20:35 PDT
I should have read comment 16.
Comment 29 Olli Pettay [:smaug] 2009-06-14 09:55:34 PDT
Created attachment 383180 [details] [diff] [review]
strict subset of the previous patch

No need for the nsINode flag.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2009-06-14 14:55:48 PDT
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.
Comment 31 Olli Pettay [:smaug] 2009-06-14 15:02:53 PDT
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
Comment 32 Samuel Sidler (old account; do not CC) 2009-06-14 18:25:30 PDT
Comment on attachment 383180 [details] [diff] [review]
strict subset of the previous patch

Approved for 1.9.0.12. a=ss
Comment 33 Olli Pettay [:smaug] 2009-06-15 00:29:33 PDT
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
Comment 34 Al Billings [:abillings] 2009-06-30 17:43:23 PDT
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.
Comment 35 Martin Stránský 2009-07-08 03:04:26 PDT
Does not affect 1.8.

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