Bug 86376

Summary: Incorrect cloning behavior for scripts: "already started" flag not copied
Product: WebKit Reporter: Boris Zbarsky <bzbarsky>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, koivisto, pf, rniwa, webkit.review.bot
Priority: P2 Keywords: EasyFix
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
testcase
none
Patch none

Boris Zbarsky
Reported 2012-05-14 08:35:01 PDT
Created attachment 141736 [details] testcase See attached testcase. It should say "PASS". The relevant spec is at http://www.whatwg.org/specs/web-apps/current-work/#already-started and says: The cloning steps for script elements must set the "already started" flag on the copy if it is set on the element being cloned.
Attachments
testcase (286 bytes, text/html)
2012-05-14 08:35 PDT, Boris Zbarsky 		no flags
Details
Patch (6.01 KB, patch)
2012-05-14 17:27 PDT, Pablo Flouret 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-05-14 14:33:01 PDT
This should be easy to fix.
Boris Zbarsky
Comment 2 2012-05-14 15:52:48 PDT
One other note: ideally the SVG behavior would be fixed along with the HTML one, though of course the HTML spec can't require behavior for SVG scripts...
Pablo Flouret
Comment 3 2012-05-14 17:27:14 PDT
Created attachment 141824 [details] Patch
Pablo Flouret
Comment 4 2012-05-14 17:37:10 PDT
I missed the SVG comment, i'll see if i can make a testcase and fix if needed.
Pablo Flouret
Comment 5 2012-05-15 15:35:58 PDT
There's already http://trac.webkit.org/browser/trunk/LayoutTests/svg/dom/SVGScriptElement/script-clone-rerun.svg?format=txt which tests that "already started" is copied over when the element is cloned, but the bug here wasn't in the cloning code itself, it was just observable through it. I'm not very proficient in svg, so if anyone thinks the actual bug here could also be an issue in svg i'd appreciate a testcase or a pointer on how to make one.
Boris Zbarsky
Comment 6 2012-05-15 15:55:22 PDT
There's a testcase at https://bugzilla.mozilla.org/attachment.cgi?id=621309 that shows an <svg:script> running after being imported from a DOMParser-generated document. I assume the attached patch fixes that situation for <html:script>, right?
Pablo Flouret
Comment 7 2012-05-15 17:45:43 PDT
(In reply to comment #6) > There's a testcase at https://bugzilla.mozilla.org/attachment.cgi?id=621309 that shows an <svg:script> running after being imported from a DOMParser-generated document. > > I assume the attached patch fixes that situation for <html:script>, right? WebKit doesn't support text/html in DOMParser yet, afaik. But that testcase does indeed fail for svg and xhtml. Implementation-wise, the xml case is a bit trickier to fix since DOMParser uses Document::createElement, which uses the automatically generated element constructors. Also, is there even enough info at that point to decide whether 'already started' should be set or not? Adam, any ideas? Btw, should there also be a note for the xml case here: http://html5.org/specs/dom-parsing.html#domparser
Boris Zbarsky
Comment 8 2012-05-15 18:24:45 PDT
If I read the specs right, the "already started" flag is set whenever the element is inserted into a document... That should include DOMParser and XMLHttpRequest automatically. By the way, WebKit seems to fail the similar XMLHttpRequest testcase: test.xml: <root> <script xmlns="http://www.w3.org/1999/xhtml"> document.body.textContent = 'FAIL'; </script> </root> test.html: <!DOCTYPE html> <body>PASS <script> var xhr = new XMLHttpRequest(); xhr.open("GET", "test.xml", false); xhr.send(); document.body.appendChild( document.importNode(xhr.responseXML.querySelector("script"), true)); </script> </body>
WebKit Review Bot
Comment 10 2012-05-18 12:11:41 PDT
Comment on attachment 141824 [details] Patch Clearing flags on attachment: 141824 Committed r117611: <http://trac.webkit.org/changeset/117611>
WebKit Review Bot
Comment 11 2012-05-18 12:11:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.