Last Comment Bug 127016 - [FIXr]appendChild(cloneNode(true)) causes inline scripts to execute again
: [FIXr]appendChild(cloneNode(true)) causes inline scripts to execute again
Status: RESOLVED FIXED
: fixed1.7
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P3 normal with 1 vote (vote)
: mozilla1.7final
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-02-21 08:03 PST by Frank Baxter
Modified: 2008-07-31 02:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple test case to show table row cloning problem (831 bytes, text/html)
2002-02-21 08:05 PST, Frank Baxter
no flags Details
Testcase (1.03 KB, text/html)
2002-02-21 17:02 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
script file for testcase (20 bytes, application/x-javascript)
2002-02-21 17:36 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Testcase using external script (1011 bytes, text/html)
2002-02-21 17:37 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Well, this should be sufficient, right? (872 bytes, patch)
2004-02-13 12:15 PST, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
jst: superreview+
Details | Diff | Splinter Review
Patch updated to comments (1009 bytes, patch)
2004-03-29 19:33 PST, Boris Zbarsky [:bz] (still a bit busy)
asa: approval1.7+
Details | Diff | Splinter Review

Description Frank Baxter 2002-02-21 08:03:37 PST
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 2002-02-21 08:05:24 PST
Created attachment 70718 [details]
Simple test case to show table row cloning problem
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2002-02-21 13:09:01 PST
"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 2002-02-21 15:15:53 PST
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 [:bz] (still a bit busy) 2002-02-21 16:52:36 PST
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 [:bz] (still a bit busy) 2002-02-21 17:02:56 PST
Created attachment 70829 [details]
Testcase

I tried this in IE5.0/Solaris and in Mozilla.  Mozilla executes the script, IE
does not.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2002-02-21 17:06:57 PST
Timeless just tested this with IE6/Windows.  That also does not run the script.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2002-02-21 17:08:31 PST
What about script elements that refere to external scripts?
Comment 8 Jesse Ruderman 2002-02-21 17:20:13 PST
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 [:bz] (still a bit busy) 2002-02-21 17:36:04 PST
Created attachment 70836 [details]
script file for testcase
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2002-02-21 17:37:46 PST
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).
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2002-02-21 17:45:43 PST
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, jst@mozilla.com) 2002-02-21 18:37:04 PST
I vote for compatibility with IE here, since that does actually mean quite a lot
nowadays...
Comment 13 Jesse Ruderman 2002-02-21 19:43:22 PST
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 2002-02-22 11:26:55 PST
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 [:bz] (still a bit busy) 2002-02-22 11:40:23 PST
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 2002-02-22 12:20:01 PST
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, jst@mozilla.com) 2003-03-23 12:55:26 PST
Mass-reassigning bugs to dom_bugs@netscape.com
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-02-13 09:11:47 PST
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 [:bz] (still a bit busy) 2004-02-13 12:15:32 PST
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?
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-02-18 16:06:47 PST
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 2004-02-19 16:09:53 PST
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 [:bz] (still a bit busy) 2004-02-19 16:32:16 PST
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 [:bz] (still a bit busy) 2004-03-13 17:21:13 PST
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) 2004-03-17 15:09:13 PST
> 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 2004-03-17 18:22:16 PST
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, jst@mozilla.com) 2004-03-29 17:10:22 PST
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 [:bz] (still a bit busy) 2004-03-29 19:33:13 PST
Created attachment 145065 [details] [diff] [review]
Patch updated to comments
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2004-03-29 19:33:39 PST
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 [:bz] (still a bit busy) 2004-03-29 19:35:41 PST
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] 2004-04-07 15:35:32 PDT
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 [:bz] (still a bit busy) 2004-04-09 16:21:30 PDT
Checked in for 1.7

Note You need to log in before you can comment on or make changes to this bug.