352260 - XBL binding failing to get document in getBoxObject

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[John Gaunt (redfive)]

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.

Comment 1

[John Gaunt (redfive)]

Attachment: 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 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[John Gaunt (redfive)]

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.

Comment 4

[John Gaunt (redfive)]

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 )

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[John Gaunt (redfive)]

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? 

Comment 7

[John Gaunt (redfive)]

Attachment: version 1 with comments added.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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 blocking1.8.1.1 on the bug...

Comment 9

[Mike Schroepfer]

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 1.8.1.1.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 239394 [details] [diff] [review]
version 1 with comments added.

approved for 1.8 branch, a=dveditz for drivers

Comment 11

[John Gaunt (redfive)]

checked in 1.8 branch 11/30

Comment 12

[Scott MacGregor]

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)