Closed Bug 575462 Opened 11 years ago Closed 11 years ago

Crash [@ nsDocument::AddToIdTable] with mutation events


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

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: jruderman, Assigned: mounir)



(Keywords: crash, regression, testcase)

Crash Data


(3 files, 1 obsolete file)

Regression from bug 572614?
Attached file stack trace+
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
OS: Mac OS X → All
Assignee: nobody → jonas
blocking2.0: --- → final+
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?
Assignee: jonas → mounir.lamouri
Jonas, why do we have to lie about NODE_HAS_ID? IOW, why UnsetFlags(NODE_HAS_ID) isn't done just after RemoveFromIdTable() ?
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)
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)
> 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?
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #471369 - Flags: review?(jonas)
Comment on attachment 471369 [details] [diff] [review]
Patch v1

This patch is breaking two tests:

Attachment #471369 - Flags: review?(jonas)
Attached patch Patch v2Splinter Review
This patch might be better (at least, it's fixing the two tests): clearing the flag in AfterSetAttr instead of BeforeSetAttr.
Attachment #471369 - Attachment is obsolete: true
Attachment #471610 - Flags: review?(jonas)
> 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.
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.
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.
Pushed without HasAttr (IOW, I pushed the attached patch).
Closed: 11 years ago
Flags: in-testsuite+
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Someone running the 2010926 build crashed in this stack:
(In reply to comment #15)
> Someone running the 2010926 build crashed in this stack:

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?
Filed Bug 600594 for the crash in Comment 15.
Crash Signature: [@ nsDocument::AddToIdTable]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.