The default bug view has changed. See this FAQ.

XBL binding failing to get document in getBoxObject

RESOLVED FIXED

Status

()

Core
XBL
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: John Gaunt (redfive), Assigned: John Gaunt (redfive))

Tracking

({fixed1.8.1.1})

1.8 Branch
x86
Linux
fixed1.8.1.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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

11 years ago
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.
Assignee: general → mozilla
Status: NEW → ASSIGNED
Attachment #238350 - Flags: review?(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...
Attachment #238350 - Flags: superreview+
Attachment #238350 - Flags: review?(bzbarsky)
Attachment #238350 - Flags: review+
(Assignee)

Comment 3

11 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

11 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 )
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

11 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

11 years ago
Created attachment 239394 [details] [diff] [review]
version 1 with comments added.
Attachment #238350 - Attachment is obsolete: true
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

11 years ago
Attachment #239394 - Flags: approval1.8.1?

Comment 9

11 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

11 years ago
Blocks: 357052
(Assignee)

Updated

11 years ago
Attachment #239394 - Flags: approval1.8.1.1?
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

11 years ago
checked in 1.8 branch 11/30
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 363163

Comment 12

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