Should we throw WRONG_DOCUMENT_ERROR when moving nodes between documents
Jonas Sicking
discussion to the platform newsgroup)
Hi All,
As I'm sure many of you know we in gecko1.9 have made a change to start
throwing WRONG_DOCUMENT_ERROR exceptions when a node is moved between
documents without first calling .adoptNode on it.
For a while now we've had a debate weather we should revert that change
or not since it has turned out that throwing this exception breaks
websites. Starting this thread so that we can come to a final conclusion
on the subject.
Below is a summary by peterv of the options:
##
* Keep current behaviour, require adoptNode/importNode when inserting a
node into a document that's not the node's ownerDocument
- Code that uses adoptNode (falling back to importNode) will work in
FF 3, Safari and Opera. It will also work in FF 2 once we fix
adoptNode to not throw on the branch.
- Code that doesn't use adoptNode/importNode will work in FF < 3,
Opera > 9.5 and Safari(*). It will not work in FF 3, IE or
older versions of Opera and Safari(*).
- Code that creates nodes in the right document right away will
work everywhere.
* Change behaviour, don't require adoptNode/importNode
- Code that uses adoptNode/importNode will work in IE, FF 3, Safari
and Opera. It will also work in FF 2 once we fix adoptNode to not
throw on the branch.
- Code that doesn't use adoptNode/importNode will work in FF,
Opera > 9.5 and Safari(*). It will not work in IE, Safari(*) or
older versions of Opera.
- Code that creates nodes in the right document right away will
work everywhere.
(*) Safari does throw WRONG_DOCUMENT_ERR if a node was inserted in a
document before.
##
Throwing makes us compatible with both IE and the spec. The advantage of
this is that someone writing code for Firefox will not have to fix that
code up in order to run in IE. It is true that they will likely have
make other changes to get things working in IE anyway, but every such
thing is a hassle.
On the other hand, once they do realize that they need to fix this in
IE, it is usually possible to write the code such that it works in both
IE and Firefox, i.e. they wouldn't have to do browser in the code, they
just need to test cross browser.
Generally it's a no-brainer if we can be compatible with both IE and the
spec. However it seems like there are a number of pages already that
have gecko-only codepaths that do this. How many is as usual hard to
guess. There is a bug filed on tracking websites that have broken over this:
https://bugzilla.mozilla.org/show_bug.cgi?id=407636
Another thing to note is that WRONG_DOCUMENT_ERROR exceptions aren't
always due to this change. There are other cases where that exception is
thrown too. So if you run into this exception don't assume that it's
this error.
My opinion is that I think we should revert the fix. While it sucks to
break compat with the spec, I think the spec isn't really adding any
value here. And we're not making it harder for other specs to progress.
It seems to me like we're adding more compatibility woes for developers
(FF2->FF3) than we are reducing them (FF3->IE).
Another thing to note is that we wouldn't be backing out all the work
that peterv put into this. The majority of it dealt with cleaning up the
code that deals with nodes moving between documents. That code used to
be very spread out and buggy, it is now centralized and works much better.
Also note that this isn't about the ability to move nodes between
documents. That will always be possible using the document.adoptNode
function.
How do other people feel?
/ Jonas
Martijn
> How do other people feel?
I opt for the:
* Change behaviour, don't require adoptNode/importNode
For me, it seems the regressions that were caused by this requirement
of adoptNode/importNode thing, are too painful and it just doesn't
seem like they are getting fixed quick enough.
Regards,
Martijn
> / Jonas
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
--
Martijn Wargers - Help Mozilla!
http://weblogs.mozillazine.org/qa/
http://wiki.mozilla.org/Mozilla_QA_Community
irc://irc.mozilla.org/qa - /nick mw22
Johnny Stenback
checkout being the latest) that I'm in favor of reverting this change.
Had we done this 5 years ago I bet we could've gotten away with this,
but now I think it's simply too late :(
--
jst
Boris Zbarsky
> * Change behaviour, don't require adoptNode/importNode
>
> - Code that uses adoptNode/importNode will work in IE, FF 3, Safari
> and Opera. It will also work in FF 2 once we fix adoptNode to not
> throw on the branch.
I'm still trying to pin down the behavior various browsers have here. Safari
and Opera used to do what the spec says, then switched to doing what we used to
do because of compat problems with sites that were maybe authored for Gecko, right?
What does IE actually do? I've seen conflicting descriptions, at least in part
because IE's behavior seems to depend on what the original and new
ownerDocuments are.
As I understand IE's behavior:
* Moving a node from one HTML document to another is never allowed
* Moving a node from one XML document (e.g. result of XMLHttpRequest) to
another XML document is allowed without any importNode/adoptNode calls.
* Not sure about moving between HTML and XML; might be never allowed, since
the two use different DOM implementations. But this needs testing!
* I don't know whether IE implements adoptNode/importNode, and if it does
whether they do anything.
* IE's behavior might not be the same in IE6 and IE7, and might also depend
on the MSXML version.
Would someone be willing to put together some tests for this stuff and see what
IE actually does? I feel like I've brought this up numerous times, and not once
have we done this... Such tests would be useful to us too, as regression tests.
> Throwing makes us compatible with both IE and the spec.
That's the thing. I'm not sure it actually makes us completely compatible with
IE, and it seems like knowing just how compatible (or not) it makes us is a
prereq for the rest of this discussion.
Unless of course we figure that we want to go back to the 2.x behavior no matter
how compatible the current behavior is with IE, I guess....
> There is a bug filed on tracking websites that have broken over this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=407636
Two questions I have here:
1) Are the sites basically breaking because the expect XMLHttpRequest
documents to be treated a certain way? Or are they really moving
nodes between HTML documents?
2) Have we made an effort to contact said sites?
To answer the actual question raised in the mail, I suspect that the spec's
requirement for adopt/import probably goes back to the idea of multiple
interacting DOM implementations (as in IE). Just moving nodes between DOM impls
might be impossible, forcing an adopt/import. On the other hand, the DOM
application shouldn't need to know which documents come from which
implementation, so the adopt/import was just made mandatory for all document
changes. jst, do you recall whether this was actually the reasoning?
Those reasons don't apply much on the Web nowadays, except in IE. That said,
are there security benefits for preventing accidental automatic scope (and hence
principal) changes for nodes?
If there are, we might want to consider some sort of behavior where
auto-importing is OK if the XPConnect scope or the principal or something
doesn't change, and require import otherwise. The question is whether there are
such security benefits (I don't know), whether that solution would cover the
currently-breaking use cases (I suspect it would), and whether it wouldn't be
too inconsistent from authors' point of view (I suspect it might).
My $0.07,
Boris
biju
with a about:config item handleerror.wrong_document_error ='warning'
this will allow the QA person to set
handleerror.wrong_document_error='error' for testing to check for spec
compliance of JS code.
An annoyed developer can set handleerror.wrong_document_error='none'
for debugging other errors in his code.
This will allow time for companies to fix their web pages.
And in a future version of firefox we should just turn it to 'error',
that way firefox will follow the spec.
wgiano...@gmail.com
This would seem to be the most sensible approach. I have always
thought the browser should have a developer mode or something that
would produce error pop-ups for things that might cause cross-browser
issues. And a normal or user mode that is tolerant of such things so
as not to break sites that would otherwise work.
Peter Van der Beken
> - Code that creates nodes in the right document right away will
> work everywhere.
> Throwing makes us compatible with both IE and the spec. The advantage of
> this is that someone writing code for Firefox will not have to fix that
> code up in order to run in IE. It is true that they will likely have
> make other changes to get things working in IE anyway, but every such
> thing is a hassle.
I think that this is the one reason to keep the change. If we want to
encourage developers to write standards-compliant cross-browser code we
should throw in Firefox too, and document/evangelize the best practice
which is creating the node in the right document.
> Generally it's a no-brainer if we can be compatible with both IE and the
> spec. However it seems like there are a number of pages already that
> have gecko-only codepaths that do this. How many is as usual hard to
> guess. There is a bug filed on tracking websites that have broken over
> this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=407636
The problem is that we don't know what that list is worth. If it
represents a good overview of what breaks, we can be pretty sure that
the sites that run into this will be fixed fairly quickly, since most
are modern maintained web applications.
I'm still for not backing this out.
Peter
Jonas Sicking
> Jonas Sicking wrote:
>> * Change behaviour, don't require adoptNode/importNode
>>
>> - Code that uses adoptNode/importNode will work in IE, FF 3, Safari
>> and Opera. It will also work in FF 2 once we fix adoptNode to not
>> throw on the branch.
>
> I'm still trying to pin down the behavior various browsers have here.
> Safari and Opera used to do what the spec says, then switched to doing
> what we used to do because of compat problems with sites that were maybe
> authored for Gecko, right?
>
> What does IE actually do? I've seen conflicting descriptions, at least
> in part because IE's behavior seems to depend on what the original and
> new ownerDocuments are.
>
> As I understand IE's behavior:
>
> * Moving a node from one HTML document to another is never allowed
> * Moving a node from one XML document (e.g. result of XMLHttpRequest) to
> another XML document is allowed without any importNode/adoptNode calls.
> * Not sure about moving between HTML and XML; might be never allowed, since
> the two use different DOM implementations. But this needs testing!
> * I don't know whether IE implements adoptNode/importNode, and if it does
> whether they do anything.
> * IE's behavior might not be the same in IE6 and IE7, and might also depend
> on the MSXML version.
Yes, this is how I understand it too. Moving to/from HTML documents
never work, moving between two XML documents work without calling
adoptNode/importNode.
> Would someone be willing to put together some tests for this stuff and
> see what IE actually does? I feel like I've brought this up numerous
> times, and not once have we done this... Such tests would be useful to
> us too, as regression tests.
I guess I can do some testing here, though I'm not really sure it
matters much. Even if we say that IE always throws when moving nodes
between documents I think the arguments are strong that we shouldn't at
this point. And if IE in certain cases doesn't throw, that only enforces
that conclusion.
The other thing that is hard to gauge is that even if IE doesn't throw
in some cases, it's very hard to tell how much those cases are used.
>> Throwing makes us compatible with both IE and the spec.
>
> That's the thing. I'm not sure it actually makes us completely
> compatible with IE, and it seems like knowing just how compatible (or
> not) it makes us is a prereq for the rest of this discussion.
>
> Unless of course we figure that we want to go back to the 2.x behavior
> no matter how compatible the current behavior is with IE, I guess....
That's what I think the conclusion is.
>> There is a bug filed on tracking websites that have broken over this:
>> https://bugzilla.mozilla.org/show_bug.cgi?id=407636
>
> Two questions I have here:
>
> 1) Are the sites basically breaking because the expect XMLHttpRequest
> documents to be treated a certain way? Or are they really moving
> nodes between HTML documents?
The most common case seems to be using the wrong document to create a
node, and then insert that node into a document.
> 2) Have we made an effort to contact said sites?
Yes, we have contacted several sites. Many have fixed their code, but I
there are still ones that haven't. Yahoo mail originally broke, then got
fixed, then released a new beta version that is broken again, and that
still haven't been fixed.
And there are still more sites trickling in, such as google checkout
being discovered having this problem the other day. It's unknown weather
these are sites that stopped working recently since they don't test with
FF3 yet, or if it's because no-one has tested these sites yet.
> To answer the actual question raised in the mail, I suspect that the
> spec's requirement for adopt/import probably goes back to the idea of
> multiple interacting DOM implementations (as in IE). Just moving nodes
> between DOM impls might be impossible, forcing an adopt/import. On the
> other hand, the DOM application shouldn't need to know which documents
> come from which implementation, so the adopt/import was just made
> mandatory for all document changes. jst, do you recall whether this was
> actually the reasoning?
Jst says: It was originally designed that way because in other
environments, such as Java, you could actually have multiple different
DOM implementations and there it wouldn't necessarily be possible to
move nodes between documents.
> Those reasons don't apply much on the Web nowadays, except in IE. That
> said, are there security benefits for preventing accidental automatic
> scope (and hence principal) changes for nodes?
Since adoptNode exists, we support moving nodes between documents
anyway. So no, there are no such benefits.
> If there are, we might want to consider some sort of behavior where
> auto-importing is OK if the XPConnect scope or the principal or
> something doesn't change, and require import otherwise. The question is
> whether there are such security benefits (I don't know), whether that
> solution would cover the currently-breaking use cases (I suspect it
> would), and whether it wouldn't be too inconsistent from authors' point
> of view (I suspect it might).
Lets keep things simple. IMHO we should either always throw, or never
throw. Whatever weird rules we come up with we're going to have to live
with for a long long time. I think authors will thank us for that too.
/ Jonas
Mike Shaver
> > 2) Have we made an effort to contact said sites?
>
> Yes, we have contacted several sites. Many have fixed their code, but I
> there are still ones that haven't. Yahoo mail originally broke, then got
> fixed, then released a new beta version that is broken again, and that
> still haven't been fixed.
One case that we hit recently was a web interface on some HP (I think)
hardware; it'll be challenging to get that updated, especially since
we still haven't shipped a Fx2 update in which adoptNode is anything
but exception bait. (Can't find the bug number now, because
Bugzilla's down, but I think it was 412550?)
The only point I've heard in favour of keeping this check is to ease
the burden of people who develop for Firefox, then test/fix/test/fix
to get it working on IE. That's a decent number of people, to be
sure, but I think they'd be about as well served by a warning to the
error console (for all the good that's done with CSS).
Mike
Boris Zbarsky
> The other thing that is hard to gauge is that even if IE doesn't throw
> in some cases, it's very hard to tell how much those cases are used.
Somewhat; we've gotten a bug report or two on them. Not that widely, though, I
agree.
> The most common case seems to be using the wrong document to create a
> node, and then insert that node into a document.
So this is definitely the non-IE (or Gecko-specific) codepath, since this
doesn't work in IE, right? :(
> Yahoo mail originally broke, then got
> fixed, then released a new beta version that is broken again
That's just unfortunate....
> Jst says: It was originally designed that way because in other
> environments, such as Java, you could actually have multiple different
> DOM implementations and there it wouldn't necessarily be possible to
> move nodes between documents.
OK, so what I was guessing.
>> Those reasons don't apply much on the Web nowadays, except in IE.
>> That said, are there security benefits for preventing accidental
>> automatic scope (and hence principal) changes for nodes?
>
> Since adoptNode exists, we support moving nodes between documents
> anyway.
That's not quite the same. A script calling adoptNode is explicitly adopting a
node into some Document. A script just appending a node, without necessarily
knowing where it came from, might be sticking a "dangerous" node into the
Document...
That said, I'm having a hard time seeing security benefits here as well.
> Lets keep things simple. IMHO we should either always throw, or never
> throw.
Simple is good. ;) I was just offering another option in case we were unhappy
enough with both keeping the patch as-is and going back to not requiring
adopt/import.
Have we considered raising this issue with the wepapi WG or whatever, by the way?
-Boris
Jonas Sicking
>> The most common case seems to be using the wrong document to create a
>> node, and then insert that node into a document.
>
> So this is definitely the non-IE (or Gecko-specific) codepath, since
> this doesn't work in IE, right? :(
Yup.
>> Yahoo mail originally broke, then got fixed, then released a new beta
>> version that is broken again
>
> That's just unfortunate....
And this will likely keep happening until FF3 is out the door and sites
start testing with it. Ideally they should test their sites before FF3
is actually released, but that doesn't always happen.
>> Lets keep things simple. IMHO we should either always throw, or never
>> throw.
>
> Simple is good. ;) I was just offering another option in case we were
> unhappy enough with both keeping the patch as-is and going back to not
> requiring adopt/import.
There is one more alternative, and that is to do what safari does. They
don't let you move nodes from one document to another, but they do let
you create a node with one document and then insert it into another.
I.e. they will implicitly adopt, but only if the node has never been
inserted into a document.
However I see little benefit with such a solution.
> Have we considered raising this issue with the wepapi WG or whatever, by
> the way?
It has been raised. The result was to not change anything since the spec
is quite clear and it's used by other things than browsers.
/ Jonas
rocal...@gmail.com
> The only point I've heard in favour of keeping this check is to ease
> the burden of people who develop for Firefox, then test/fix/test/fix
> to get it working on IE. That's a decent number of people, to be
> sure,
For what it's worth --- that's almost every Web developer of public-
facing sites that I meet.
Rob