Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla1.7final

Status

()

Core
DOM: Core & HTML
P3
normal
RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: Frank Baxter, Assigned: bz)

Tracking

({fixed1.7})

Trunk
mozilla1.7final
fixed1.7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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...
(Reporter)

Comment 1

16 years ago
Created attachment 70718 [details]
Simple test case to show table row cloning problem
(Assignee)

Comment 2

16 years ago
"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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: cloneNode(true) doesn't work correctly when children contain scripts → cloneNode(true) doesn't work correctly when children contain scripts (scripts are executed)
(Reporter)

Comment 3

16 years ago
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.
(Assignee)

Comment 4

16 years ago
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.
(Assignee)

Comment 5

16 years ago
Created attachment 70829 [details]
Testcase

I tried this in IE5.0/Solaris and in Mozilla.  Mozilla executes the script, IE
does not.
Attachment #70718 - Attachment is obsolete: true
(Assignee)

Comment 6

16 years ago
Timeless just tested this with IE6/Windows.  That also does not run the script.
What about script elements that refere to external scripts?

Comment 8

16 years ago
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.
(Assignee)

Comment 9

16 years ago
Created attachment 70836 [details]
script file for testcase
(Assignee)

Comment 10

16 years ago
Created attachment 70837 [details]
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).
(Assignee)

Comment 11

16 years ago
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.
I vote for compatibility with IE here, since that does actually mean quite a lot
nowadays...

Comment 13

16 years ago
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.
OS: Linux → All
Hardware: PC → All
Summary: cloneNode(true) doesn't work correctly when children contain scripts (scripts are executed) → appendChild(cloneNode(true)) causes inline scripts to execute again
(Reporter)

Comment 14

16 years ago
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?
(Assignee)

Comment 15

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

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

Updated

16 years ago
Priority: -- → P3
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
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.
(Assignee)

Comment 19

14 years ago
Created attachment 141349 [details] [diff] [review]
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?
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 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
Attachment #141349 - Flags: review+
(Assignee)

Comment 22

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

Will make that change.	jst, is this ok with you?
Attachment #141349 - Flags: superreview?
(Assignee)

Comment 23

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

Apparently I failed to type in jst's email....
Attachment #141349 - Flags: superreview? → superreview?(jst)
> 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.
QA Contact: stummala → ian
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 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.
Attachment #141349 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 27

14 years ago
Created attachment 145065 [details] [diff] [review]
Patch updated to comments
Assignee: general → bzbarsky
Attachment #141349 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 28

14 years ago
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...
Attachment #145065 - Flags: approval1.7?
(Assignee)

Comment 29

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

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

Updated

14 years ago
Summary: appendChild(cloneNode(true)) causes inline scripts to execute again → [FIXr]appendChild(cloneNode(true)) causes inline scripts to execute again
Target Milestone: --- → mozilla1.7final

Comment 30

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

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #145065 - Flags: approval1.7? → approval1.7+

Updated

14 years ago
Keywords: fixed1.7
(Assignee)

Comment 31

14 years ago
Checked in for 1.7
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.