Closed
Bug 599588
Opened 14 years ago
Closed 14 years ago
Scripts created using createContextualFragment should be executable
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: t.brain, Assigned: hsivonen)
References
Details
(Whiteboard: [already landed, shows up on blocker queries due to Bugzilla bug][has patch][not a final blocker])
Attachments
(3 files, 2 obsolete files)
608 bytes,
text/html
|
Details | |
16.76 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
17.02 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; SLCC1; .NET CLR 2.0.50727; .NET CLR 3.0.30729; .NET CLR 3.5.30729; .NET4.0C) Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 Upto Firefox 3.6, SCRIPT tags could be applied via innerHTML, as long as the element you applied it to wasn't already part of the document and was appended later. Additionally, SCRIPT tags could also be appended using Range.createContextualFragment and appended to the document. Reproducible: Always Steps to Reproduce: Use the following code: var o=document.createElement("div"); o.innerHTML="<script>alert(1);<\/script>"; document.documentElement.appendChild(o); Or the following code: var rg=document.createRange(); document.documentElement.appendChild(rg.createContextualFragment("<script>alert(1);<\/script>")); Actual Results: Nothing. Expected Results: Show an alert (works upto version 3.6).
Comment 1•14 years ago
|
||
I believe the new behavior is what the HTML5 spec calls for.
Be that as it may, this is a breaking change to existing content. I should also note that this "workaround" to get SCRIPT tags working via innerHTML is documented across the web, for example: http://domscripting.com/blog/display/99 http://poeticcode.wordpress.com/2007/10/03/innerhtml-and-script-tags/
I have to agree with comment #2. Why not change the behavior only for HTML5 documents (with the HTML5 doctype) if it is indeed a standardization issue?
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #1) > I believe the new behavior is what the HTML5 spec calls for. It indeed is. (In reply to comment #2) > Be that as it may, this is a breaking change to existing content. You aren't citing an actual existing site that's breaking. Do I guess correctly that you are a Web developer and you have used this trick on your own site and you aren't a user who is seeing breakage on someone else's site? (In reply to comment #3) > I have to agree with comment #2. > > Why not change the behavior only for HTML5 documents (with the HTML5 doctype) > if it is indeed a standardization issue? That's not going to be the solution here. It would be bad for maintainability (see http://weblogs.mozillazine.org/roc/archives/2008/01/post_2.html) and it would align badly with Mozilla's mission (see http://lists.w3.org/Archives/Public/public-html/2007Apr/0279.html).
Assignee | ||
Comment 5•14 years ago
|
||
The innerHTML case is clearly WONTFIX: In http://hsivonen.iki.fi/test/moz/scripts/innerHTML-not-in-doc.html the script doesn't run in Firefox trunk, Opera 10.63, any WebKit-based browser or in IE9 beta1. Ms2ger, do you have an opinion on createContextualFragment? Results for http://hsivonen.iki.fi/test/moz/scripts/createContextualFragment.html : API not supported: IE9 Script runs: Firefox 3.6 and Opera 10.63 Script doesn't run: Firefox trunk and any WebKit-based browser
Comment 6•14 years ago
|
||
I think I'd prefer to keep the same code path for innerHTML and createContextualFragment, but I don't mind changing. (At least, if you convince Hixie to provide a hook I can use.) I assume you tested before-HTML5-parsing WebKits too? Reporter, do you know of any pages that rely on scripts executing from createContextualFragment?
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #6) > I think I'd prefer to keep the same code path for innerHTML and > createContextualFragment, but I don't mind changing. Keeping createContextualFragment stuff non-runnable would easier for me, though I could make the case why it's less logical. > I assume you tested before-HTML5-parsing WebKits too? I did. FWIW, XSLT-created fragments run in Gecko and Opera but not in WebKit: http://hsivonen.iki.fi/test/moz/scripts/XSLTProcessor-transformToFragment.html Sicking, what do we want to do about XSLT-created fragments going forward? It would be slightly odd for them to have different runnability from createContextualFragment.
Assignee | ||
Comment 8•14 years ago
|
||
Marking this NEW, since comment 0 is correct about what the behavior is.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I should note that with IE, the behavior is a little different but the innerHTML not in doc testcase does reproduce if it's slightly tweaked. IE requires the SCRIPT tag to have the defer attribute, and that the innerHTML contains other content besides the SCRIPT tag, in which case the SCRIPT does run. The attached testcase shows this behavior, and runs under IE8 and IE9 beta.
Reporter | ||
Comment 10•14 years ago
|
||
It's also worth noting that WebKit has an open bug for the createContextualFragment case: https://bugs.webkit.org/show_bug.cgi?id=12234
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #9) > that the innerHTML > contains other content besides the SCRIPT tag, in which case the SCRIPT does > run. Thanks. I didn't realize other content was required. Indeed http://hsivonen.iki.fi/test/moz/scripts/innerHTML-in-doc-defer.html doesn't run in IE but http://hsivonen.iki.fi/test/moz/scripts/innerHTML-in-doc-defer-plus-cruft.html does.
Comment 12•14 years ago
|
||
We should probably block on this one way or another (whether fixing or wontfixing).
blocking2.0: --- → ?
Here are my opinions: I strongly feel like .innerHTML created <script>s should never run. I.e. I don't want to replicate any weird IE behavior here with regards to defer. I less strongly feel like the XSLTProcessor.transformToFragment *should* allow scripts to run. transformToFragment is generally used when creating new content, and if someone goes out of the way to create <script>s, then they probably want them run. (innerHTML on the other hand is often used to copy existing content between various places in the page). There are also no complicated "when does the script run" questions, as the script for sure won't run until the author inserts the fragment using the DOM. I don't really have strong feelings one way or another on createContextualFragment. My gut feeling is that it's more like .innerHTML/.outerHTML/.insertAdjacentHTML which none of them run <script>s. So I would lean towards not running scripts. But if someone can site use cases or significant site breakage, I'm all ears.
Hmm.. Given that there are two bugs filed (one on webkit, and this one) on createContextualFragment producing non-running <script>s, filed by two different people. And given that createContextualFragment is a lot like transformToFragment in that it returns a fragment without complex "when do scripts run" questions. And given that createContextualFragment generally doesn't have the same a.innerHTML += x; or a.innerHTML = b.innerHTML; usage patterns, it might make more sense to allow <script>s produced using that API to run.
Assignee | ||
Comment 15•14 years ago
|
||
The top use case for createContextualFragment that I'm aware of is emulating insertAdjacentHTML in engines like Gecko that haven't gotten around to implementing insertAdjacentHTML. So in that sense, it would be best to make createContextualFragment prevent script execution. OTOH, if one tries to think of createContextualFragment as a standalone feature, it's easy to think of it as a shortcut for building a fragment manually, and if one built a fragment manually, the scripts would be runnable. (I think the innerHTML part should still be a clear WONTFIX. I'm OK with XSLTProcessor.transformToFragment creating runnable scripts.)
(In reply to comment #15) > OTOH, if one tries to think of createContextualFragment as a standalone > feature, it's easy to think of it as a shortcut for building a fragment > manually, and if one built a fragment manually, the scripts would be runnable. Right, that's how I was thinking. If we've gotten away with letting scripts be runnable so far, then I think we could keep doing it. There's no reason not to implement insertAdjecentHTML for next version (in fact, there was no reason not to implement it for this version, just fell between the cracks)
Assignee | ||
Comment 17•14 years ago
|
||
T. Brains, when you reported this bug, did you have a use case that would have been better addressed by createContextualFragment creating executable scripts? f yes, what was the use case? Note that if we made createContextualFragment create fragmentts with executable scripts, inserting the fragments to the document wouldn't guarantee the execution order between external scripts or between external and internal scripts.
Assignee | ||
Comment 19•14 years ago
|
||
Per voice discussion with sicking, the plan is to make createContextualFragment-created scripts runnable.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #486898 -
Flags: review?(jonas)
Attachment #486898 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 21•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/108a3a3878c1 The createContextualFragment part of the original report is FIXED and the innerHTML part is WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: SCRIPT tags no longer work when applied via innerHTML → Scripts created using createContextualFragment should be executable
Assignee | ||
Comment 22•14 years ago
|
||
Ms2ger pointed out to me that I forgot to revise the IID of nsIParser. (This is an interface that extensions shouldn't be touching, so extension compat shouldn't be an issue.)
Attachment #490052 -
Flags: review?(bzbarsky)
I don't know what you mean by "extensions shouldn't be touching". We know they very frequently touch "internal" interfaces, and they are often forced to due to lack of blessed external ones. So I think we should treat this as a interface change like any other and post to relevant people (probably the .platform list)
Comment 24•14 years ago
|
||
Indeed. If extensions aren't touching the interface, the IID doesn't matter. If they are, the patch that was pushed will break the if they're calling the relevant function. "should" is not relevant here. If it's not unacceptable performance wise, we should just add a new interface for the new method...
Assignee | ||
Comment 25•14 years ago
|
||
(In reply to comment #24) > Indeed. If extensions aren't touching the interface, the IID doesn't matter. I see. > If they are, the patch that was pushed will break the if they're calling the > relevant function. "should" is not relevant here. If extensions do call the function, there's a problem, since the entire nsIParser interface is tightly coupled with other internal stuff and is really inappropriate for extension use. > If it's not unacceptable performance wise, we should just add a new interface > for the new method... Does it need to be a full XPCOM interface or is it enough to be able to static_cast to it? It's always known what the concrete class behind the interface is.
Comment 26•14 years ago
|
||
If you always know the concrete class, then static casting to that class should be fine, imo. In fact, we should consider moving away from using nsIParser and just using nsParser directly in gklayout...
Assignee | ||
Comment 27•14 years ago
|
||
(In reply to comment #26) > If you always know the concrete class, then static casting to that class should > be fine, imo. I added a fully abstract class in between, because casting to the concrete class would have exposed many more .h files that code outside the HTML5 parser are not supposed to see/use. > In fact, we should consider moving away from using nsIParser and just using > nsParser directly in gklayout... There are now 2 classes that implement nsIParser. I expect we'll going to continue to need some kind of superclass for the different parsers going forward, since even if the XML parser starts using the HTML5 tree op mechanism, it most likely will make sense for the XML IO driver and the HTML5 IO driver classes not to be the same class.
Attachment #490052 -
Attachment is obsolete: true
Attachment #490052 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 28•14 years ago
|
||
Attachment #490532 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #490534 -
Flags: review?(bzbarsky)
Comment on attachment 490534 [details] [diff] [review] Introduce a new non-XPCOM interface just for the fragment parser entry point, with all files This is ok with me, but why introduce nsAHtml5FragmentParser? Why not cast to nsHtml5Parser instead?
Attachment #490534 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 30•14 years ago
|
||
(In reply to comment #29) > Comment on attachment 490534 [details] [diff] [review] > Introduce a new non-XPCOM interface just for the fragment parser entry point, > with all files > > This is ok with me, Thanks. > but why introduce nsAHtml5FragmentParser? Why not cast to > nsHtml5Parser instead? As mentioned in comment 27, exposing nsHtml5Parser.h to where it would need to be cast would no longer maintain a clear separation of what's meant to be used outside the HTML5 parser source directory and what's not. Now only very limited .h files are exported. http://hg.mozilla.org/mozilla-central/rev/9ce8d67eaccb
> As mentioned in comment 27, exposing nsHtml5Parser.h to where it would need to
> be cast would no longer maintain a clear separation of what's meant to be used
> outside the HTML5 parser source directory and what's not. Now only very limited
> .h files are exported.
I don't see such separation as a goal. We've gone down that path before in gecko, and it lead to absurdities such as nsINodeInfo and nsIParser.
Would very much appreciate a followup patch that removes the new abstract base class.
Assignee | ||
Comment 32•14 years ago
|
||
Filed bug 612839 about the follow-up abstract class removal.
Updated•13 years ago
|
Whiteboard: [hardblocker]
Updated•13 years ago
|
Whiteboard: [hardblocker] → [hardblocker] [has patch]
Assignee | ||
Comment 33•13 years ago
|
||
This has landed already but a Bugzilla caching bug makes this show up as non-FIXED on queries occasionally.
Comment 34•13 years ago
|
||
(In reply to comment #33) > This has landed already but a Bugzilla caching bug makes this show up as > non-FIXED on queries occasionally. I am seeing this occur as well.
Comment 35•13 years ago
|
||
(In reply to comment #33) > This has landed already but a Bugzilla caching bug makes this show up as > non-FIXED on queries occasionally. Clearing [hardblocker] from whiteboard to "deal" with this for the time being. If this bug gets re-opened, it needs to grow its [hardblocker] back.
Whiteboard: [hardblocker] [has patch] → [has patch]
Updated•13 years ago
|
Whiteboard: [has patch] → [has patch][not a final blocker]
Assignee | ||
Updated•13 years ago
|
Whiteboard: [has patch][not a final blocker] → [already landed, shows up on blocker queries due to Bugzilla bug][has patch][not a final blocker]
You need to log in
before you can comment on or make changes to this bug.
Description
•