Jesse Ruderman Reporter
	Description • 19 years ago

To reproduce, just load the testcase.

TB16527704X shows what might be an incomplete stack trace:

nsContentIterator::NextNode
nsContentIterator::Next
nsRange::OwnerChildRemoved   
nsGenericElement::doRemoveChildAt   
nsGenericElement::RemoveChildAt   
nsGenericElement::doRemoveChild   
nsGenericElement::RemoveChild   
XPCWrappedNative::CallMethod

	Jesse Ruderman Reporter
	Comment 2 • 19 years ago

I can reproduce on Windows 2003, Windows XP, and Mac.

	Jesse Ruderman Reporter
	Comment 3 • 19 years ago

I wonder why web pages are able to see the anonymous content that makes up the missing-plugin placeholder.  I also wonder whether making that anonymous content inaccessible to web pages would be considered a complete fix for this bug.

	Martijn Wargers (dead)
	Comment 4 • 19 years ago

I think this is an xbl bug. I don't think there is currently a way to make xbl anonymous content inaccessible, maybe there should be a way? (it would fix the testcase, I guess, but not the bug itself).

	Jesse Ruderman Reporter
	Comment 5 • 19 years ago

I forgot that the missing-plugin placeholder uses XBL now.  I should try to make a testcase that doesn't involve plugins, but instead supplies its own XBL.

Is this a dup of bug 205735?  (Would the change suggested in bug 205735 comment 34 fix this crash?)

	Martijn Wargers (dead)
	Comment 6 • 19 years ago

Before the crash, I get some assertions:
"###!!! ASSERTION: invalid removeIndex: 'removeIndex >= 0 && !
(oldParent == container && removeIndex == insPos)', file c:/mozilla/mozilla/cont
ent/base/src/nsGenericElement.cpp, line 3028")

###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc()', f
ile c:/mozilla/mozilla/content/base/src/nsGenericDOMDataNode.cpp, line 665

###!!! ASSERTION: Already have a parent.  Unbind first!: '!GetParent() || aParen
t == GetParent()', file c:/mozilla/mozilla/content/base/src/nsGenericDOMDataNode
.cpp, line 669

and more.
Backtrace of some of the assertions included in the file.

	Martijn Wargers (dead)
	Comment 7 • 19 years ago

I think somewhere here it is already going wrong. When the anonymous node is move out of the xbl, the old node is not removed.
I thought that bug 308120 made it impossible to move anonymous xbl nodes out of xbl.
But it seems like comment nodes or textnodes as direct children of the xbl binding are not treated as xbl anonymous content, so you don't get the NS_ERROR_DOM_NOT_SUPPORTED_ERR error. You can also see this in the dom inspector where those nodes turn up as black, while normal xbl anonymous nodes turn up as red (I always used to wonder about that).

So I don't think this is a dup of bug 205735.

	Martijn Wargers (dead)
	Comment 8 • 19 years ago

I guess that's what the XXX comment is about:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericDOMDataNode.cpp#678
"// XXXbz we don't keep track of the binding parent yet.  We should."

	Martijn Wargers (dead)
	Comment 9 • 19 years ago

Well, this fixes the crash by making those nodes really anonymous.
But it isn't any good because it also makes direct text/commentnodes of the bound element anonymous, which isn't true.

	Bengt-Erik Soderstrom
	Comment 10 • 19 years ago

BTW, all java as asked are (and were) OK for me.
/be

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 19 years ago

> I wonder why web pages are able to see the anonymous content

That's just how XBL works, sadly.

That said, in this case we'll end up with a bogus removeIndex, since our bindingParent is bogus.  Perhaps for safety sake we should simply throw if removeIndex ends up -1?  That would fix this bug and all...  We could even remove the bindingParent check bug 308120 added -- since the kid is anonymous, it's not removable, no matter how it came to be anonymous...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 19 years ago

Jonas, should we land this on branches too (like bug 308120)?  If so, I'll write a 1.8 branch version of this patch....

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 13 • 19 years ago

Comment on attachment 216996 [details] [diff] [review]
Per comment 11

Yeah, we want this for branches too sadly. You should probably wait until i've landed bug 325730 on the branches though.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 19 years ago

Sounds good.  I'll ask you for approval and so forth... ;)

Fixed on trunk.

	Tim Riley [:timr]
	Comment 15 • 19 years ago

Crasher.  a=timr for drivers.

	Darin Fisher
	Comment 16 • 19 years ago

Please land this on the MOZILLA_1_8_BRANCH soon, so it can bake before we accept it for the MOZILLA_1_8_0_BRANCH.  Please add the "fixed1.8.1" keyword when it has been checked in on the MOZILLA_1_8_BRANCH.  Thanks!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 19 years ago

> Please land this on the MOZILLA_1_8_BRANCH soon

I can't until bug 325730 lands on that branch.  See the dep list.

	Daniel Veditz [:dveditz]
	Comment 18 • 18 years ago

Comment on attachment 216996 [details] [diff] [review]
Per comment 11

approved for 1.8.0 branch, a=dveditz for drivers

	Tracy Walker [:tracy]
	Comment 19 • 18 years ago

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

no crash, however testcase2 shows red (failed).

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 18 years ago

Yep.  That should have gotten fixed on trunk today by bug 329122.

	Jesse Ruderman Reporter
	Comment 21 • 17 years ago

I checked in my testcase as a crashtest.

Should Martijn's "testcase2" be a mochitest?  It seems a little hacky to use reftest for JavaScript-set "pass/fail" messages.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 17 years ago

That looks like a good mochitest candidate, yes.  Want to do it?

	Jesse Ruderman Reporter
	Comment 23 • 17 years ago

Well, I don't know how to use mochitest... ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 17 years ago

There are docs in devmo, I think...  Please let me know if they're unclear and I'll help.

	Jesse Ruderman Reporter
	Comment 25 • 17 years ago

This is my first mochitest.  It's based on Martijn's testcase2, but tests that getBindingParent returns the specific div instead of just testing that it's not null.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 17 years ago

Some comments on the test:

* use addLoadEvent(init) instead of an onload attr on the body.  That will
  guarantee in-order firing of load stuff, basically.
* Add a description string to you is() calls.  Something like:

    is(document.getBindingParent(document.getAnonymousNodes(t)[0]), t,
       "Wrong binding parent for anonymous node");

  and perhaps "Wrong binding parent for anonymous node child" for the other
  test.  That message will be shown if the test fails.

Other than those nits, looks good!

	Jesse Ruderman Reporter
	Comment 27 • 17 years ago

Added diagnostic messages and switched to addLoadEvent as requested by bz.  Also added two more tests and a comment about why we're waiting for onload.

	Jesse Ruderman Reporter
	Comment 28 • 17 years ago

Mochitest checked in.