Last Comment Bug 352260 - XBL binding failing to get document in getBoxObject
: XBL binding failing to get document in getBoxObject
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Core
Classification: Components
Component: XBL (show other bugs)
: 1.8 Branch
: x86 Linux
: -- normal (vote)
: ---
Assigned To: John Gaunt (redfive)
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on: 363163
Blocks: songbird
  Show dependency treegraph
 
Reported: 2006-09-11 18:42 PDT by John Gaunt (redfive)
Modified: 2007-02-08 15:20 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
version 1, fixes my immediate errors (721 bytes, patch)
2006-09-13 19:40 PDT, John Gaunt (redfive)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
version 1 with comments added. (1.55 KB, patch)
2006-09-20 12:25 PDT, John Gaunt (redfive)
mtschrep: approval1.8.1-
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review

Description John Gaunt (redfive) 2006-09-11 18:42:16 PDT
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) 2006-09-13 19:40:17 PDT
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 2 Boris Zbarsky [:bz] (still a bit busy) 2006-09-13 19:58:07 PDT
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) 2006-09-14 11:09:11 PDT
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) 2006-09-14 12:41:06 PDT
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 [:bz] (still a bit busy) 2006-09-20 10:12:56 PDT
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) 2006-09-20 12:14:50 PDT
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) 2006-09-20 12:25:21 PDT
Created attachment 239394 [details] [diff] [review]
version 1 with comments added.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2006-09-20 14:00:48 PDT
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 2006-09-21 10:15:48 PDT
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] 2006-11-29 11:42:00 PST
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) 2006-11-30 11:18:37 PST
checked in 1.8 branch 11/30
Comment 12 Scott MacGregor 2006-12-22 08:05:38 PST
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)

Note You need to log in before you can comment on or make changes to this bug.