409111 - WRONG_DOCUMENT_ERR on unattached nodes through getBoxObjectFor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Matthew Mastracci]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.5) Gecko/20070718 Fedora/2.0.0.5-1.fc7 Firefox/2.0.0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b3pre) Gecko/2007121904 Minefield/3.0b3pre

Previous versions of Firefox would allow document.getBoxObjectFor() on unattached DOM nodes.  Recent nightly builds of Firefox3 incorrectly throw WRONG_DOCUMENT_ERR in this.

alert(document.getBoxObjectFor(document.createElement('div')));



Reproducible: Always

Steps to Reproduce:
1.  View testcase

Actual Results:  
WRONG_DOCUMENT_ERR thrown

Expected Results:  
BoxObject should be returned

Comment 1

[Matthew Mastracci]

Attachment: Testcase for bug

Comment 2

[Matthew Mastracci]

Added blocking bugs.

Note that pages built with the Google Web Toolkit will encounter this error when trying to get a box object for an unattached DOM node.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I'll write a patch for this tomorrow.

Comment 4

[Boris Zbarsky [:bzbarsky]]

This was a purposeful change, for what it's worth.  See also bug 340084.

I find it highly unfortunate that Google Web Toolkit is using this, especially for nodes not in a document.  That's never been particularly safe (as the bug history shows).  Have you filed a bug on Google Web Toolkit?

Requesting blocking, but I feel that we should not change trunk behavior from what it is now unless there are _really_ good reasons to and someone manages to come up with a very very safe patch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

In particular, the ownership model here is pretty different from branch, iirc, so I'm not sure the branch patch would work.  There would have to be some pretty serious convincing to convince me that it does.

Comment 6

[Matthew Mastracci]

If it's decided that this bug shouldn't be fixed, I can file a bug against GWT that could likely be fixed in the next point release of GWT 1.4 and be released with GWT 1.5.

A simple workaround in the GWT code would be wrapping calls to getBoxObjectFor with a try/catch and returning zero values if the exception is thrown.  In all fairness, there's also a bug in the GWT Tree component that's assuming that the GWT widget is attached the the DOM tree.  It just happens to not fail in an obvious way with a {0,0,0,0} boxModel object.

I posted a message to the GWT -devel list describing the problem:

http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_frm/thread/dd4eb7938faad3a5#

Comment 7

[Boris Zbarsky [:bzbarsky]]

I wonder why this is using box objects anyway...  I don't expect them to remain indefinitely exposed to unprivileged code.  Would using bounding client rects be a better idea, or do they not do what this code is trying to do?

Comment 8

[Matthew Mastracci]

GWT implements a getAbsoluteTop and getAbsoluteLeft method which are designed to get the screen position of an element.  These are used for various positioning tasks and for adjusting event coordinates in Gecko (with significant hacks to make it work in early Gecko).  It only ever uses the screenX and screenY properties of the box object.

getBoundingClientRects() would work, but since the current client code is shared between all Gecko1.8+ browsers right now, I believe that it likely won't change until getBoxObjectFor is removed outright, or concrete plans are made.  

If this issue is a WONTFIX (I'm not entirely convinced it really is a bug myself :)), perhaps changing the error to NOT_SUPPORTED_ERR or INVALID_STATE_ERR would be better to avoid confusion.  

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why do you need screen coords exactly?  Client coords (clientTop, etc) should be just fine unless popup menus are involved....  And even then in Gecko 1.9, I believe.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

The good reason the make this work is not to break web apps.
This patch is what I proposed in bug 340084 comment 23, it makes
boxobject "working" as long as element and its ownerdocument are alive and
element isn't adopted to a new document.
Bz, feel free to r+sr=+/-

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix also xulelement.boxObject

Consistency is very important. No reason to handle xulElement.boxObject in a different way.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> The good reason the make this work is not to break web apps.

How common is use of boxObjects on stuff not in the document by web apps?  What will they do when we remove boxObjects for untrusted content (which we should)?

As for the patch, how does this change the ownership model?  Is the new setup documented sowewhere?  How does it differ from branch?  What does the leak situation look like?

I'm not sure I can review this without a very clear description of why this patch is ok; I just don't have the time to read through all the bug history and current code here trying to reconstruct what the heck is going on.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> How common is use of boxObjects on stuff not in the document by web apps?  What
> will they do when we remove boxObjects for untrusted content (which we should)?
We could add a warning to 1.9, see bug 409220.
As far as I know, getBoxObject is used quite often. Not sure how often when
element isn't in document. But this bug and Bug 340084 indicate that it is
being used.

> As for the patch, how does this change the ownership model?  Is the new setup
> documented sowewhere?  How does it differ from branch?  What does the leak
> situation look like?
Without the patch document has strong pointers to boxobjects' elements, with 
the patch those change to raw pointers. Deleting or adopting an element or 
deleting document takes care of clearing the boxobject. Without the patch it 
is up to Unbinding/DestroyContent to clear boxObjects.
I can't see any leaks, nor do I expect any leaks. Whenever element is deleted or adopted, or document deleted, boxObject is cleared.
 
> I'm not sure I can review this without a very clear description of why this
> patch is ok; I just don't have the time to read through all the bug history and
> current code here trying to reconstruct what the heck is going on.
Rephrasing what I already said: the current boxObject handling is simple; 
document owns box objects and unbind/destroyContent clears them. The patch 
changes that so that document doesn't own the boxObject.element and 
deleting/adopting an element or deleting document clears boxObject.

Comparing to 1.8
(1) on 1.8 moving an element to a new document clears boxobject.
    (the adoptNode case in the patch)
(2) on 1.8 deleting a document clears all the boxobjects.
    (The patch does the same)
(3) on 1.8 document has strong pointer to the boxObject.element.
    If an element has a boxObject, the element can't die before unbinding or 
    deleting document in which case box object is cleared because of (1) or (2).
    (The patch changes document to have raw pointers to boxobject.element, but
     that pointer is cleared whenever (1) or (2) or element deletion happens)
(4) on 1.8 unbinding an element clears boxobject. There isn't actually
    any reason for this because of (1), (2) and (3)

Comment 14

[Rajeev Dayal]

(In reply to comment #9)
> Hmm.  Why do you need screen coords exactly?  Client coords (clientTop, etc)
> should be just fine unless popup menus are involved....  And even then in Gecko
> 1.9, I believe.
> 

Hi Boris,

As Matt mentioned, GWT uses getBoxObjectFor's screen coordinates to determine an element's absoluteLeft and absoluteTop positions.

The reason that GWT uses boxObject.screenX/screenY is related to bugs #356066 (and possibly #307502). These issues prevent us from using boxObject.x and boxObject.y. Interestingly enough, these problems with boxObject.x/y do not exist in pre-1.8 versions of Gecko. 

The workaround that we stumbled on was to compute the absoluteLeft by using document.getBoxObjectFor(elem).screenX - document.getBoxObjectFor(document.documentElement).screenX. We computed absoluteTop in a similar fashion. In pre-1.8 versions of Gecko, screenX and screenY do not take scrolling into account, so we had to add additional code to correct for this.

Would clientTop/clientLeft work in this situation? Do they suffer from the same bugs with boxObject.x/y and offsetLeft/offsetTop mentioned above?

If all else fails, we could introduce a deferred binding target for FF3 that uses getBoundingClientRects.


Thanks,
Rajeev

Comment 15

[Boris Zbarsky [:bzbarsky]]

> These issues prevent us from using boxObject.x and boxObject.y.

Are you using HTML?  If so, what's wrong with element.clientLeft and element.clientTop?

> Do they suffer from the same bugs with boxObject.x/y and offsetLeft/offsetTop
> mentioned above?

I can't tell, since neither bug is clear enough to say what the problem, if any, is...  As far as I know, clientLeft/clientTop should work fine.

Comment 16

[Boris Zbarsky [:bzbarsky]]

smaug, I tried to sort though this stuff, and maybe I'm being really stupid but I can't manage to convince myself that your patch is safe.  I'll try to look again, but that might take several weeks.  You might want to try another reviewer...

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I don't see anything particularly unsafe with boxobjects, not currently nor with the patch.
Jst and sicking are on vacation already, I think.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 294039 [details] [diff] [review]
fix also xulelement.boxObject

Peter, perhaps you have time?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why do we want to do this? I realize that not "fixing" this bug will break some websites, but it seems like they are depending on some very quirky behavior. What size should the boxObject for an element not in the document give? I think any size would seem like a lie since the element isn't rendered at all.

Returning null or throwing seems like the right thing to do.

Ideally, this isn't an API that websites should use at all.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #19)
> Why do we want to do this? I realize that not "fixing" this bug will break some
> websites,
The patch does fix this by making the ownership rules of box object somewhat
saner that currently. (No need for those XUL specific hacks)

> but it seems like they are depending on some very quirky behavior.
Nothing quirky, IMO.

> What size should the boxObject for an element not in the document give?
0,0

> I think
> any size would seem like a lie since the element isn't rendered at all.
Similar "issue" with getBoundingClientRect()
document.createElement('div').getBoundingClientRect().top == 0
So I think box object returning 0 isn't that bad.

> Ideally, this isn't an API that websites should use at all.
Right, but because it is used and the fix really isn't that complicated... 

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Re-requesting blocking1.9?
When this functionality was broken in the branch, we got quite fast complaints
about it.
Comment 13 explains pretty clearly how the patch works.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So I'm not convinced that getBoxObjectFor is something that sites should be using. Do the new APIs we have cover the functionality people are using? If so I don't think we need to even support this function for web pages as it'll just make it harder for other browsers.

Marking this a blocker, but I don't necessarily think that the correct way to fix it is to use the attached patch.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

But do we want to break more websites than absolutely necessary with 1.9?
Perhaps adding a warning when using getBoxObjectFor on HTMLDocument?
(Um, l10n-freeze was already some time ago.)

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If we ever plan to remove getBoxObjectFor the more we wait the harder it will be. So I would prefer to do it as soon as we have a decent story for alternative APIs for people to use.

If we wait for one more release it will just be even more people that we break once we do remove it.

Comment 25

[Jeff Walden [:Waldo]]

Wasn't getBoundingClientRect (?) the API that was supposed to replace getBoxObjectFor?  Why haven't we disabled getBoxObjectFor in web content in favor of it yet?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #25)
> Why haven't we disabled getBoxObjectFor in web content
I guess some remote XUL still depends on boxObjects, though hopefully
those are using xulElement.boxObject (, which is something the patch fix too)

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Or let's say so that if we ship getBoxObjectFor in 1.9, it should work as well as
possible and definitely not have this bug.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, I think I agree with that.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

I don't think we can remove .getBoxObject, not this late.
Sites are using it and getBoundingClientRect was introduced in gecko 1.9, so
sites would be forced to use them both in order to support FF2 and FF3.
So, could we fix this, and perhaps add warning to console if .getBoxObject is
used with non-xul-document.
Other browsers support getBoundingClientRect (right?) so sites should be
slowly moving to use that.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Regardless what we decide, this should be at least P2.

Comment 32

[Neil Deakin]

(In reply to comment #25)
> Wasn't getBoundingClientRect (?) the API that was supposed to replace
> getBoxObjectFor?

No, it replaces only the four of the properties of the boxObject: x,y,width,height The remaining properties have no other equivalent.

In this particular case (Google Web Toolkit) they are using screenX/screenY to work around some bugs. It is possible however that getBoundingClientRect doesn't have those bugs and it can be used instead.

Comment 33

[Mike Schroepfer]

Moving to P1 as we've gotten more reports of site incompatibility for B4.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 294039 [details] [diff] [review]
fix also xulelement.boxObject

Lets just do this.

It would be nice to warn in the console when people are using this though and point people to the new cross-browser API.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with warning

"if possible" is there after all. 
getBoundingClientRect doesn't always replace boxObjects

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 303133 [details] [diff] [review]
with warning

Ideally we'd only warn once per document or some such, to avoid flooding the console (which could hurt perf).

Also, you might want to mention in the message that we might remove the function from future releases.

r/sr=me either way.

Comment 37

[Reed Loden [:reed]]

Comment on attachment 303133 [details] [diff] [review]
with warning

You need explicit approval by endgame drivers for late-l10n changes.

Comment 38

[Mike Schroepfer]

Comment on attachment 303133 [details] [diff] [review]
with warning

a+ schrep - let's get this in

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

I'll add a boolean flag to document so that the warning happens only
once per document.

I think "deprecated" should be clear enough for web developers.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with flag

I'll check in this.