I get the following error: WARNING: NS_ENSURE_TRUE(nsDoc) failed, file /builds/xulrunner/mozilla/content/xul/content/src/nsXULElement.cpp, line 2593 Looking at that line there are comments about wether we should be calling getCurrentDocument(); which is the current way, or getOwnerDocument(). There are similar comments elsewhere in the file. Adding a check for a null doc and a subsequent call to getOwnerDocument() removed the warning didn't appear to have any side effect on Songbird. Talked with bz in #developers and he said the checke in nsXULElement should check for the same document as gets checked in nsDocument::GetBoxObjectFor(). That check looks for the owner doc.
Created attachment 238350 [details] [diff] [review] version 1, fixes my immediate errors I'm sure there are more places, but this simple patch fixed my errors. Should I go back through and swap /all/ calls over? This one was pretty obvious since it called GetCurrentDoc() and GetBoxObjectFor() checks against GetOwnerDoc(). I'm happy to do some more here if someone can lay out what parameters are for switching the calling convention over.
Comment on attachment 238350 [details] [diff] [review] version 1, fixes my immediate errors Add a comment here saying this should match what GetBoxObjectFor checks? And a similar comment in GetBoxObjectFor pointing to this code? r+sr=bzbarsky with those. Other than that, no need to look at other places unless you really want to... at some point we'll do that when we eliminate GetDocument. Part of the problem at the moment is that in a lot of cases it's really not clear which doc we should be using...
Hmmm.... I figured this had to land on the trunk first, but when I look there the check in nsDocument.cpp is against GetCurrentDoc(), not GetOwnerDoc() as it is on the 1.8 branch. Which way is "the future"? Should I just apply for branch approval? Maybe the fix should be to change the check in nsDocument.cpp to check against GetCurrentDoc() instead of changing nsXULElement.
Nope, the issues is that GetCurrentDoc() is returning null for that XULElement, so switching both to use the current doc. didn't work ( on the 1.8 branch, in xulrunner for songbird )
So the problem is that we have a long-standing inconsistency in our handling of boxobjects -- depending on how you get to a given DOM state, you might or might not be able to get a useful box object. On branch, we're maintaining that inconsistency. On trunk, we have fixed it. So on trunk, the rule is simple. If the node is not in a document, it has no box object. See bug 340084 comment 21...
Ok, I read bug 340084 and bug 330818. So I understand a little more of what is happening. Which makes me think I'm getting this error when I shouldn't. And it certainly doesn't seem to be affecting the functionality of the program. I'm going to do some more testing to see if I can track down exactly what element we're accessing the box object. However, It certainly seems like there is an inconsistency in the branch since XULElement gets the document one way and then the document runs it's check against a different call. So my thought would be that the v1 patch would be good for the branch and I should request approval. I do that by setting the flag on blocking1.8.1 right?
Created attachment 239394 [details] [diff] [review] version 1 with comments added.
Yeah, the branch is definitely inconsistent and should be fixed. I think you can request approval1.8.1 on the patch. If that gets granted, you land on branch. If not, you request blocking126.96.36.199 on the bug...
Comment on attachment 239394 [details] [diff] [review] version 1 with comments added. We are frozen for RC1 and this is not a topcrash, security issue, or major regression - so it will have to wait until 188.8.131.52.
Comment on attachment 239394 [details] [diff] [review] version 1 with comments added. approved for 1.8 branch, a=dveditz for drivers
checked in 1.8 branch 11/30
Hey John, it's starting to look like this patch caused some thunderbird CPU pegging issues on the 1.8.1 branch according to Kent, any ideas on why that would be? This change seems pretty innocent. (See Bug #363163)