Closed
Bug 352260
Opened 18 years ago
Closed 18 years ago
XBL binding failing to get document in getBoxObject
Categories
(Core :: XBL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mozilla, Assigned: mozilla)
References
Details
(Keywords: fixed1.8.1.1)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mtschrep
:
approval1.8.1-
dveditz
:
approval1.8.1.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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•18 years ago
|
||
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...
Attachment #238350 -
Flags: superreview+
Attachment #238350 -
Flags: review?(bzbarsky)
Attachment #238350 -
Flags: review+
Assignee | ||
Comment 3•18 years ago
|
||
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.
Assignee | ||
Comment 4•18 years ago
|
||
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•18 years ago
|
||
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...
Assignee | ||
Comment 6•18 years ago
|
||
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?
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #238350 -
Attachment is obsolete: true
Comment 8•18 years ago
|
||
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...
Assignee | ||
Updated•18 years ago
|
Attachment #239394 -
Flags: approval1.8.1?
Comment 9•18 years ago
|
||
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.
Attachment #239394 -
Flags: approval1.8.1? → approval1.8.1-
Assignee | ||
Updated•18 years ago
|
Attachment #239394 -
Flags: approval1.8.1.1?
Comment 10•18 years ago
|
||
Comment on attachment 239394 [details] [diff] [review]
version 1 with comments added.
approved for 1.8 branch, a=dveditz for drivers
Attachment #239394 -
Flags: approval1.8.1.1? → approval1.8.1.1+
Assignee | ||
Comment 11•18 years ago
|
||
checked in 1.8 branch 11/30
Comment 12•18 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•