127016 - [FIXr]appendChild(cloneNode(true)) causes inline scripts to execute again

Status: RESOLVED FIXED

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

Description

[Frank Baxter]

When attempting to clone a table row whose TD contains a java script, the
browser goes into a seemingly infinite reload state.  The URL changes to
wyciwyg://0... and the browser can only recover by stopping, going back, then
reloading the page.  I found this problem with the daily build for 2002-02-19.

I tested this under IE 5.0/5.5 and it works correctly.  Here's the clone code I
used:

   var rowNode = document.getElementById('Row1');
   var bodyNode = document.getElementById('EditTable');
  
   var newRow = rowNode.cloneNode(true);
   bodyNode.appendChild(newRow);

I will attach a full test case...

Comment 1

[Frank Baxter]

Attachment: Simple test case to show table row cloning problem

Comment 2

[Boris Zbarsky [:bzbarsky]]

"The URL changes to wyciwyg://0... and the browser can only recover by stopping,
going back, then reloading the page."

This is correct behavior assuming that we execute the script....

I feel that we should not execute <script> nodes if they are appended to content
from the DOM.

Comment 3

[Frank Baxter]

If we do not execute the script - then how will the table data be generated? 
The test case I added was simple, but the real problem that I'm trying to solve
involves a complex javascript to generate the table data.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The table data is already in the DOM as children of the table cell node (as is
the script).

I'm working on a testcase that will actually test what Mozilla and IE do here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

I tried this in IE5.0/Solaris and in Mozilla.  Mozilla executes the script, IE
does not.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Timeless just tested this with IE6/Windows.  That also does not run the script.

Comment 7

[Johnny Stenback (:jst)]

What about script elements that refere to external scripts?

Comment 8

[Jesse Ruderman]

This bug is invalid.  Creating and inserting a new script node *should* run the
script.  If I clone an entire document into a new window, I expect scripts to
work in the new window, which often requires that inline scripts defining
functions have been executed.  document.write may not play nicely with DOM 2,
but we shouldn't reduce the power and completeness of the DOM 2 functions to
accommodate the DOM 0 function document.write.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: script file for testcase

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase using external script

This includes an external script.  The script _is_ run in IE5 (at cloneNode()
time too, not when it's reinserted into the document).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Jesse says IE6 acts just like IE5 on the second testcase.

I have to say that we should just make a decision here....  Do we try to emulate
the IE behavior for scripts with no <src>?  There are some issues here which
Jesse has raised, especially with scripts that define things instead of creating
content, and especially if the document being inserted into is different than
the document being taken from.

Frank, for purposes of real-world application, I'd suggest simply removing the
script node from the DOM in the script itself.  That way you'll get identical
behavior to what you have now in IE (which doesn't run the script anyway) and
will get identical behavior between IE and Mozilla.

Comment 12

[Johnny Stenback (:jst)]

I vote for compatibility with IE here, since that does actually mean quite a lot
nowadays...

Comment 13

[Jesse Ruderman]

After talking to bz, I realized that if I'm trying to clone an arbitrary page,
it makes more sense to copy the variables and functions over than it does to
have each of the scripts execute again.  I no longer think this bug is invalid.

Comment 14

[Frank Baxter]

Boris,

Removing the script node works fine.  I didn't realize that table data and the
script were both there - I should have inspected it first.  Sorry about that,
I'm still learning this.  I guess the real issue here is IE compatibility?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Frank, nothing to be sorry for.  :)

Yes, IE compatibility is the only issue.  Fabian, is there any way to suppress
execution of scripts without src on SetDocument, except when they are being
appended from the content sink?

I should also note that IE's behavior and ours differs for <script src=".."> and
that IE's behavior seems to me to be insane....

Comment 16

[Fabian Guisset]

bz, sorry, I don't know enough about the js engine and our script loading
mechanism to answer your question

Comment 17

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 18

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

I have a hard time deciding what I think we should do here. We don't really
follow IEs lead anyway when it comes to <script>s so IMHO IE compatibility is
not that important.

On one hand cloning a node should create a node that behaves just like the
original copy, and in this case the original copy won't be reexecuted when being
inserted again. Also, you can get the functionality by creating a new <script>
element and copying children and the src attribute.

On the other hand, cloning a script means that you're actually creating a new
scriptelement, and we do execute newly created scriptelements.

As i'm writing this i realize that i lean more towards not rerunning cloned scripts.
In the case of cloning an entire document we probably don't want to rerun
scripts (think script that do document.write). And in the case of copying part
of a DOM and inserting in the same document then we defenetly don't want to
rerun scripts.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Well, this should be sufficient, right?

Although, this doesn't handle the case when evaluation of a script clones
itself; in that case the script is _not_ yet evaluated when it's cloned, right?

Comment 20

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

Did some more thinking.

If we don't allow cloned scripts to rerun the we make it a lot harder to let
someone rerun a piece of script. They would manually have to copy all children
and attributes from an old scriptelement to the new one.

However, I can't think of any usecase where you want to rerun the same script
over and over. If you do you should put that script in a function and call the
function instead.

So the ability to clone a document without breaking seems more important then
being able to rerun a script-element.

Comment 21

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

Comment on attachment 141349 [details] [diff] [review]
Well, this should be sufficient, right?

>+  it->mIsEvaluated = mIsEvaluated;

Make that

it->mIsEvaluated = mIsEvaluated | mIsEvaluating;

Since the only way we could end up here is if the element actually contained
evaluatable script and we'll soon be setting |mIsEvaluated| to true. And add a
comment stating this.

with that, r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141349 [details] [diff] [review]
Well, this should be sufficient, right?

Will make that change.	jst, is this ok with you?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141349 [details] [diff] [review]
Well, this should be sufficient, right?

Apparently I failed to type in jst's email....

Comment 24

[Hixie (not reading bugmail)]

> However, I can't think of any usecase where you want to rerun the same script
> over and over. If you do you should put that script in a function and call the
> function instead.

One theoretical use case would be cloning the element then slightly modifying
the script then reinserting it.

But I don't feel strongly about this.

Comment 25

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

You can always do that by creating a new <script> element and clone the textnode
inside it. You can even replace the old <script> with the new one.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 141349 [details] [diff] [review]
Well, this should be sufficient, right?

Yeah, I like this with sicking's suggestion, and we should probably copy
mLineNumber in ::CloneNode() too.

sr=jst with that.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145065 [details] [diff] [review]
Patch updated to comments

Could this please be approved for 1.7?	This should be a fairly safe IE-compat
change...

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145065 [details] [diff] [review]
Patch updated to comments

Though actually, this is not quite IE compat.  See comment 18.

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 145065 [details] [diff] [review]
Patch updated to comments

a=asa (on behalf of drivers) for checkin to 1.7

Comment 31

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.7