The default bug view has changed. See this FAQ.

Scripts created using createContextualFragment should be executable

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: T. Brains, Assigned: hsivonen)

Tracking

unspecified
x86
Windows Vista
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [already landed, shows up on blocker queries due to Bugzilla bug][has patch][not a final blocker])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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).
I believe the new behavior is what the HTML5 spec calls for.
(Reporter)

Comment 2

7 years ago
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

7 years ago
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

7 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

7 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
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

7 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

7 years ago
Marking this NEW, since comment 0 is correct about what the behavior is.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 9

7 years ago
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.
(Reporter)

Comment 10

7 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

7 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.
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

7 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

7 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.
Blocking to sort this out one way or another.
blocking2.0: ? → final+
(Assignee)

Comment 19

7 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

7 years ago
Created attachment 486898 [details] [diff] [review]
Make scripts created by createContextualFragment() executable
Attachment #486898 - Flags: review?(jonas)
Attachment #486898 - Flags: review?(jonas) → review+
(Assignee)

Comment 21

7 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
Last Resolved: 7 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

7 years ago
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.)
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)
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

7 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.
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

6 years ago
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.
Attachment #490052 - Attachment is obsolete: true
Attachment #490052 - Flags: review?(bzbarsky)
(Assignee)

Comment 28

6 years ago
Created attachment 490534 [details] [diff] [review]
Introduce a new non-XPCOM interface just for the fragment parser entry point, with all files
Attachment #490532 - Attachment is obsolete: true
(Assignee)

Updated

6 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

6 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

6 years ago
Filed bug 612839 about the follow-up abstract class removal.
Depends on: 628581
Whiteboard: [hardblocker]

Updated

6 years ago
Whiteboard: [hardblocker] → [hardblocker] [has patch]
(Assignee)

Comment 33

6 years ago
This has landed already but a Bugzilla caching bug makes this show up as non-FIXED on queries occasionally.

Comment 34

6 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.
(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]
Whiteboard: [has patch] → [has patch][not a final blocker]
(Assignee)

Updated

6 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.