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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[arno renevier]

Attachment: testcase

splitted from bug #442227 comment #8

Comment 1

[arno renevier]

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 (dead)]

Maybe related to bug 395671?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I think we may want to try to sync broadcasters asyncronously, or in EndUpdate.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: delay until it is ok to synchronize

Comment 5

[[e-mail]]

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 [:bzbarsky]]

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][[e-mail]]]

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][[e-mail]]]

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][[e-mail]]]

Attachment: +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]

Comment on attachment 337899 [details] [diff] [review]
+remove listener properly from map

Seems OK. Can the testcase be incorporated?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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][[e-mail]]]

Attachment: some mochitests

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

The change to current behavior are tests 10 and 11

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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][[e-mail]]]

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 <[e-mail]>
	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]]

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][[e-mail]]]

Attachment: 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 [: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.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 for 190

Comment 24

[Daniel Veditz [:dveditz]]

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][[e-mail]]]

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][[e-mail]]]

Backed out because of leak orange.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

I should have read comment 16.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Attachment: strict subset of the previous patch

No need for the nsINode flag.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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][[e-mail]]]

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)]

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][[e-mail]]]

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 - ex-MoCo]]

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ý [:stransky] (ni? me)]

Does not affect 1.8.