Last Comment Bug 599588 - Scripts created using createContextualFragment should be executable
: Scripts created using createContextualFragment should be executable
Status: RESOLVED FIXED
[already landed, shows up on blocker ...
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: x86 Windows Vista
: -- normal with 2 votes (vote)
: ---
Assigned To: Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
:
Mentors:
Depends on: 628581
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-25 01:55 PDT by T. Brains
Modified: 2013-12-27 14:31 PST (History)
15 users (show)
hsivonen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
innerHTML not in doc, IE testcase (608 bytes, text/html)
2010-10-19 05:27 PDT, T. Brains
no flags Details
Make scripts created by createContextualFragment() executable (16.76 KB, patch)
2010-10-29 06:37 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
jonas: review+
Details | Diff | Splinter Review
Revise the IID of nsIParser (1.02 KB, patch)
2010-11-12 01:40 PST, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
no flags Details | Diff | Splinter Review
Introduce a new non-XPCOM interface just for the fragment parser entry point (13.82 KB, patch)
2010-11-15 01:19 PST, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
no flags Details | Diff | Splinter Review
Introduce a new non-XPCOM interface just for the fragment parser entry point, with all files (17.02 KB, patch)
2010-11-15 01:30 PST, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
jonas: review+
Details | Diff | Splinter Review

Description T. Brains 2010-09-25 01:55:52 PDT
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 Boris Zbarsky [:bz] 2010-09-25 02:06:46 PDT
I believe the new behavior is what the HTML5 spec calls for.
Comment 2 T. Brains 2010-09-25 02:16:40 PDT
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/
Comment 3 Ish 2010-09-25 02:29:41 PDT
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?
Comment 4 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-09-25 04:10:37 PDT
(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).
Comment 5 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-19 04:05:56 PDT
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 :Ms2ger (⌚ UTC+1/+2) 2010-10-19 04:39:41 PDT
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?
Comment 7 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-19 05:07:37 PDT
(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.
Comment 8 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-19 05:22:31 PDT
Marking this NEW, since comment 0 is correct about what the behavior is.
Comment 9 T. Brains 2010-10-19 05:27:32 PDT
Created attachment 484275 [details]
innerHTML not in doc, IE testcase

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.
Comment 10 T. Brains 2010-10-19 06:23:44 PDT
It's also worth noting that WebKit has an open bug for the createContextualFragment case: https://bugs.webkit.org/show_bug.cgi?id=12234
Comment 11 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-19 07:08:27 PDT
(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 Boris Zbarsky [:bz] 2010-10-19 08:34:14 PDT
We should probably block on this one way or another (whether fixing or wontfixing).
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2010-10-19 15:47:32 PDT
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.
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2010-10-19 15:53:33 PDT
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.
Comment 15 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-19 22:14:20 PDT
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.)
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2010-10-19 23:44:02 PDT
(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)
Comment 17 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-20 00:50:15 PDT
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.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2010-10-21 10:46:47 PDT
Blocking to sort this out one way or another.
Comment 19 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-29 03:45:27 PDT
Per voice discussion with sicking, the plan is to make createContextualFragment-created scripts runnable.
Comment 20 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-10-29 06:37:23 PDT
Created attachment 486898 [details] [diff] [review]
Make scripts created by createContextualFragment() executable
Comment 21 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-11-11 03:33:52 PST
http://hg.mozilla.org/mozilla-central/rev/108a3a3878c1

The createContextualFragment part of the original report is FIXED and the innerHTML part is WONTFIX.
Comment 22 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-11-12 01:40:17 PST
Created attachment 490052 [details] [diff] [review]
Revise the IID of nsIParser

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.)
Comment 23 Jonas Sicking (:sicking) PTO Until July 5th 2010-11-12 09:11:17 PST
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 Boris Zbarsky [:bz] 2010-11-12 09:17:35 PST
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...
Comment 25 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-11-12 09:55:10 PST
(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 Boris Zbarsky [:bz] 2010-11-12 10:01:38 PST
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...
Comment 27 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-11-15 01:19:45 PST
Created attachment 490532 [details] [diff] [review]
Introduce a new non-XPCOM interface just for the fragment parser entry point

(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.
Comment 28 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-11-15 01:30:51 PST
Created attachment 490534 [details] [diff] [review]
Introduce a new non-XPCOM interface just for the fragment parser entry point, with all files
Comment 29 Jonas Sicking (:sicking) PTO Until July 5th 2010-11-15 11:01:07 PST
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?
Comment 30 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-11-15 23:56:14 PST
(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
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2010-11-16 00:18:35 PST
> 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.
Comment 32 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2010-11-17 01:22:06 PST
Filed bug 612839 about the follow-up abstract class removal.
Comment 33 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-02-26 03:00:39 PST
This has landed already but a Bugzilla caching bug makes this show up as non-FIXED on queries occasionally.
Comment 34 Jonathan Yaniv 2011-02-26 18:57:39 PST
(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 Johnathan Nightingale [:johnath] 2011-03-01 11:06:47 PST
(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.

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