575462 - Crash [@ nsDocument::AddToIdTable] with mutation events

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Regression from bug 572614?

Comment 1

[Jesse Ruderman]

Attachment: stack trace+

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

WinXP Crash: bp-631d5f40-73a8-4538-9635-f0e952100628
Signature: nsDocument::AddToIdTable(mozilla::dom::Element*, nsIAtom*)

Frame  	Module  	Signature  	Source
0 	xul.dll 	nsDocument::AddToIdTable(mozilla::dom::Element*,nsIAtom*) 	content/base/src/nsDocument.cpp:2372
1 	xul.dll 	nsStyledElement::BindToTree(nsIDocument*,nsIContent*,nsIContent*,int) 	content/base/src/nsStyledElement.cpp:224
2 	xul.dll 	nsSVGElement::BindToTree(nsIDocument*,nsIContent*,nsIContent*,int) 	content/svg/content/src/nsSVGElement.cpp:220
3 	xul.dll 	nsINode::doInsertChildAt(nsIContent*,unsigned int,int,nsAttrAndChildArray&) 	content/base/src/nsGenericElement.cpp:3563

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sure looks like it.  At the crash point:

#2  0x11b8829c in nsDocument::AddToIdTable (this=0x1710200, aElement=0x203cc6a0, aId=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsDocument.cpp:2372
2372        mIdentifierMap.PutEntry(nsDependentAtomString(aId));

(note the null aId).  So in this case NODE_HAS_ID is true, but there's no id.  The node in this case is the one that used to have id="b" and we're inside the mutation handler.

Note that nsStyledElement::UnsetAttr removes NODE_HAS_ID _after_ nsGenericElement::UnsetAttr returns, which is after the mutation event has fired.  We should perhaps do it before the mutation event fires, from AfterSetAttr, but in any case we will have cases when NODE_HAS_ID lies (in that it returns true while the node has a null id; e.g. anything running under AttributeChanged).  So it seems like this code should deal with a null id, no?

Jonas, thoughts?

Comment 4

[Mounir Lamouri (:mounir)]

Jonas, why do we have to lie about NODE_HAS_ID? IOW, why UnsetFlags(NODE_HAS_ID) isn't done just after RemoveFromIdTable() ?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The problem is that there are 3 pieces of information that needs to be kept in sync:

* The NODE_HAS_ID flag
* The node being in the id-hash
* The attribute being in the attributes list

In particular, the last item is being handled by very generic code deep in nsGenericElement, so we can't update the other state at the same time.

Also, we can't unset the NODE_HAS_ID flag before calling into nsGenericElement::UnsetAttr as the flag needs to be set while calling notifications, otherwise layout won't know which id was just removed (there should be tests for this)

Comment 6

[Mounir Lamouri (:mounir)]

So, as Boris said, we can change the flag on nsStyledElement::AfterSetAttr but we would still be unsafe between when we actually remove the attribute and the call to AfterSetAttr. We call AttributeChanged callbacks between that.

So I guess the best thing here is to add a check in AddToIdTable. Another solution would be too ask the AddToIdTable's callers to be sure aId isn't null but I guess the crash risks doesn't worth the performance saving? (for the moment there are 3 calls but I guess we never know what may happen)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> We call AttributeChanged callbacks between that.

Those are not supposed to do anything that might trigger content script (in theory).  So that might be ok.  But I'm not sure.

> Also, we can't unset the NODE_HAS_ID flag before calling into
> nsGenericElement::UnsetAttr as the flag needs to be set while calling
> notifications, otherwise layout won't know which id was just removed (there
> should be tests for this)

This could be handled by removing in BeforeSetAttr, right?

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 471369 [details] [diff] [review]
Patch v1

This patch is breaking two tests:
base/test/test_bug564863.xhtml
layout/style/test/test_any_dynamic.html

See:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1283405601.1283406907.21938.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1283408633.1283412813.14591.gz

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

This patch might be better (at least, it's fixing the two tests): clearing the flag in AfterSetAttr instead of BeforeSetAttr.

Comment 11

[Mounir Lamouri (:mounir)]

> nsresult rv = nsGenericElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
> 
> if (isId) {
>   UnsetFlags(NODE_HAS_ID);
> }

has been changed to:

> nsMutationGuard guard;
>
> nsresult rv = nsGenericElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify);
> 
> if (isId &&
>    (!guard.Mutated(0) || !HasAttr(kNameSpaceID_None, nsGkAtoms::id))) {
>   UnsetFlags(NODE_HAS_ID);
> }

For what I understand, this tries to make sure that an id hasn't been set by an observer while removing the id, right?
I guess this could happen in AppendAttributeChanged? I don't see a reason why that could be done but technically, it might be better to check that we really hove no more id attribute before Unset the flag, right?

I'm going to run some tests with this change before pushing. Do not hesitate to yell at me if that sounds incorrect.

Comment 12

[Mounir Lamouri (:mounir)]

Hmm, just to be clear, the quoted change has been done by Jonas a few days ago. That's not what I want to do. I will just add HasAttr(kNameSpaceID_None, nsGkAtoms::id) in AfterSetAttr.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

nsGenericElement::UnsetAttr fires mutation events which means that webpage javascript can run, which could set the attribute again. So by the time we return from nsGenericElement::UnsetAttr the id attribute might have gotten set again, in which case it would be wrong to clear the id-flag.

I *think* I added tests for this.

Comment 14

[Mounir Lamouri (:mounir)]

Pushed without HasAttr (IOW, I pushed the attached patch).
http://hg.mozilla.org/mozilla-central/rev/986da52f2062

Comment 15

[Marcia Knous [:marcia]]

Someone running the 2010926 build crashed in this stack: https://crash-stats.mozilla.com/report/index/a6ecf16e-511b-49b0-9002-f7b8c2100928.

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> Someone running the 2010926 build crashed in this stack:
> https://crash-stats.mozilla.com/report/index/a6ecf16e-511b-49b0-9002-f7b8c2100928.

I do not think it's the same crash. The particularity of this bug was the RemoveAttribute. Could you open a new bug for that new crash?

Comment 17

[Marcia Knous [:marcia]]

Filed Bug 600594 for the crash in Comment 15.