419527 - "ASSERTION: Wrong root" with XBL

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: Wrong root: '!aRoot || (nsContentUtils::ContentIsDescendantOf(aStartN, aRoot) && nsContentUtils::ContentIsDescendantOf(aEndN, aRoot) && aRoot == IsValidBoundary(aStartN) && aRoot == IsValidBoundary(aEndN))', file /Users/jruderman/trunk/mozilla/content/base/src/nsRange.cpp, line 443

Comment 1

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

I'll look at this. Have to check if this is an regression from the
unconnected-ranges patch.

Comment 2

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

Not a blocker unless we can get a crash out of this. Smaug, would still be great if you can figure out what's going on.

Comment 3

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

Sure I will try to fix this once I've done with the P1 I have.
Or perhaps when I want something easier to fix.

Comment 4

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

Attachment: proposed patch

I think we want to limit re-rooting of ranges, if mRoot is bindingparent.
Passes mochitest, browser and chrome.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Please make sure to check in tests with the patch?

Comment 6

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

Attachment: same with mochitest

Comment 7

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

Comment on attachment 308597 [details] [diff] [review]
same with mochitest

This doesn't seem right when mMaySpanAnonymousSubtrees is set to true. I think what you want to do is to simply call

newRoot = IsValidBoundary(mStartParent)

And assert that that returns a non-null value.

Comment 8

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

Attachment: v2

Right, this should be enough.

Comment 9

[Jesse Ruderman]

Smaug, the presence of the mochitest in the patch means this doesn't need a crashtest, right?  (Without the patch, does the mochitest trigger the assertion?)

Comment 10

[Boris Zbarsky [:bzbarsky]]

So.. this mochitest seems to leak sometimes.  From the last leaking test run on tinderbox, the list of leaked urls includes:

  http://localhost:8888/tests/content/base/test/test_bug419527.xhtml
  http://localhost:8888/tests/content/base/test/test_bug419527.xhtml
  http://localhost:8888/tests/content/base/test/test_bug419527.xhtml
  http://localhost:8888/tests/content/base/test/test_bug419527.xhtml
  http://localhost:8888/tests/content/base/test/test_bug419527.xhtml#rangebinding

right at the end (as in, the last-created urls that leaked).  All the other ones are chrome XBL stuff or the urls secman is holding onto as part of its principals hashtable.