Closed
Bug 127016
Opened 23 years ago
Closed 20 years ago
[FIXr]appendChild(cloneNode(true)) causes inline scripts to execute again
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.7final
People
(Reporter: frank, Assigned: bzbarsky)
Details
(Keywords: fixed1.7)
Attachments
(4 files, 2 obsolete files)
1.03 KB,
text/html
|
Details | |
20 bytes,
application/x-javascript
|
Details | |
1011 bytes,
text/html
|
Details | |
1009 bytes,
patch
|
asa
:
approval1.7+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Assignee | ||
Comment 2•23 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•23 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•23 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•23 years ago
|
||
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•23 years ago
|
||
Timeless just tested this with IE6/Windows. That also does not run the script.
Comment 7•23 years ago
|
||
What about script elements that refere to external scripts?
Comment 8•23 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•23 years ago
|
||
Assignee | ||
Comment 10•23 years ago
|
||
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•23 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.
Comment 12•23 years ago
|
||
I vote for compatibility with IE here, since that does actually mean quite a lot nowadays...
Comment 13•23 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•23 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•23 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•23 years ago
|
||
bz, sorry, I don't know enough about the js engine and our script loading mechanism to answer your question
Updated•23 years ago
|
Priority: -- → P3
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•21 years ago
|
||
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•21 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•20 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)
Comment 24•20 years ago
|
||
> 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 26•20 years ago
|
||
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•20 years ago
|
||
Assignee: general → bzbarsky
Attachment #141349 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 28•20 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•20 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•20 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•20 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+
Assignee | ||
Comment 31•20 years ago
|
||
Checked in for 1.7
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•