Clinton Gormley Reporter
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0.0) Gecko/20100101 Firefox/4.0

A script tag added via innerHTML won't fire, unless it is followed by another script tag (just an opening tag required)

Reproducible: Always

Steps to Reproduce:
1. Load attached example
2. Click button "Add one script element" -> script element added, but script doesn't fire
3. Click button "Add two script elements" -> script elements added, first one fires



It doesn't matter how many script elements you add, all but the last element execute correctly.  The last element just needs an opening script tag to make the previous script fire, a closing tag by itself won't work

	Clinton Gormley Reporter
	Comment 2 • 14 years ago

Slightly simpler test case

	j.j.
	Comment 3 • 14 years ago

Should not execute at all!?
Other browsers don't, Fx3.6 did in both cases.

	Ed Morley [:emorley]
	Comment 4 • 14 years ago

Toggling html5.parser.enable makes the 3.6.x style behaviour return. Therefore caused by the new html 5 parser in Firefox 4, bug 373864.

The testcase 'doesn't work' in Chrome 12.0.712.0 dev either.

As jj said, surely this should have never worked, so this is presumably invalid?

	Clinton Gormley Reporter
	Comment 5 • 14 years ago

Agreed - to me it doesn't matter if it works one way or the other, as long as it is consistent.  We have to handle both cases anyway: browsers-that-do and browsers-that-don't.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 14 years ago

Uh... this looks broken.  Henri, what's going on?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 7 • 13 years ago

This is a security bug in the sense that a script that shouldn't run runs, which could, in theory, lead to XSS if a site was relying on spec compliance to avoid XSS. In practice, the security impact is very, very low for the time being, because any site vulnerable because of this would be vulnerable in Firefox 3.6 anyway where the behavior is even expected. Hence, not marking confidential but nominating for a point release anyway.

The bug here is that tree ops aren't flushed in the fragment case when the parser core yields due to a script, but the script execution prevention code assumes that ops get flushed when the parser core yields due to a script.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 8 • 13 years ago

Nominating for point release, because a script gets run when scripts aren't supposed to run. (Mitigating factor: Firefox 3.6 always runs scripts in that case, so sites shouldn't be relying on scripts not running.)

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 9 • 13 years ago

Now with the mochitest actually hg added.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 10 • 13 years ago

Comment on attachment 522327 [details] [diff] [review]
Flush tree ops in the fragment case when the tree builder yields due to a script, v2

The assertions I added fire when I expected them not to. I need to look at this again.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 11 • 13 years ago

The fix itself was ok. The assertions I added were bogus, so I removed the assertions.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 13 years ago

Comment on attachment 522650 [details] [diff] [review]
Flush tree ops in the fragment case when the tree builder yields due to a script, v3

r=me, but add a comment explaining why this block is needed.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 15 • 13 years ago

(In reply to comment #14)
> Comment on attachment 522650 [details] [diff] [review]
> Flush tree ops in the fragment case when the tree builder yields due to a
> script, v3
> 
> r=me, but add a comment explaining why this block is needed.

Thanks. Comment added and landed into cedar:
http://hg.mozilla.org/projects/cedar/rev/0e91598cc3f9

	(no longer active)
	Comment 16 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/0e91598cc3f9

	Daniel Veditz [:dveditz]
	Comment 17 • 13 years ago

If 3.6 was running scripts in this case then it's not clear why this can't wait for Firefox 5. If we really need it in macaw please explain.

	John Mertic
	Comment 18 • 13 years ago

It can't wait till FF 5 because it's a clear regression from FF 3.6. 

For us ( SugarCRM ) it is blocking official FF 4 support.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 13 years ago

John, what's a regression from 3.6, exactly?  In 3.6 both scripts ran.  In 4.0 only one of them runs.  In 5.0 neither one will run.

If you code is assuming the 3.6 behavior, it won't work in 5.0.  If it's not assuming that, then what _is_ it assuming?

	John Mertic
	Comment 20 • 13 years ago

The code is assuming the 3.6 behavior, which works across Safari, IE, and Chrome, but not FF 4+. 

If the plan in FF 5 for none of these scripts to run, I think many more applications will break as well.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 13 years ago

John, the behavior the HTML5 spec calls for is that neither of those scripts should run.  So yes, that's the behavior we will be shipping in Firefox 5.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 13 years ago

And for what it's worth, on the testcase attached to this bug Chrome 10 and Chrome 11 beta and Chrome 12 dev don't run the scripts.  Neither does Safari 5.  Neither does Opera 11.  In fact, Firefox 3.6 is the only browser I have here that runs the scripts in that situation (modulo the bug in Firefox 4).

So I have to assume that whatever your app is doing is completely different from what this bug is about.  Might be worth reporting a separate bug on whatever it's actually doing.

	John Mertic
	Comment 23 • 13 years ago

OK, thanks for the feedback. You are right, that our use-case was slightly different than the attached case, but for some reason we were seeing the same behavior as the original poster.

I did some testing with the latest trunk build of FF ( Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre ) and our use-case seems to work correctly there, so it sounds like we have some handling in place for script tags that works for all browsers except FF 4.. I'm guessing some form of this bug was present in FF 4 that was causing the problem seen.

Is there anyway this could be backported to FF 4? I'd be more than happy to test out a build with this fix in it to see if it fixes the problem. We'd like to be able to add FF 4 support ASAP without having to add one-off hacks in our codebase.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 13 years ago

John, I'm not sure about backporting.  It looks like there will be only one security update between Firefox 4 and the Firefox 5 release in June.

As far as testing, I pushed just this change applied on top of the Firefox 4 release changset to the try server to build a corresponding Mac release build.  Once there's a link to the resulting build, I'll post it here.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 13 years ago

John, try https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bzbarsky@mozilla.com-005ca213f301/tryserver-macosx64/firefox-4.0.en-US.mac.dmg ?

	John Mertic
	Comment 26 • 13 years ago

That seems to fix the issue :-). Thanks for creating the build for me.

If there was anyway this could make it into a security update for FF4, it would mean we could be supporting FF 4 officially. What sort of timetable would that be in?

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 13 years ago

I think there's only one security update planned for Fx4 at the moment, as I said, and that's locking down soon (matter of days).

Henri, do you think this patch is appropriate for branch?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 28 • 13 years ago

Comment on attachment 522650 [details] [diff] [review]
Flush tree ops in the fragment case when the tree builder yields due to a script, v3

(In reply to comment #27)
> Henri, do you think this patch is appropriate for branch?

Yes, I think this is appropriate for the branch. It's very low risk and it fixes pretty bogus behavior. See comment 8 for how this can be rationalized as a security fix, but see also comment 17.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 29 • 13 years ago

Note to the person processing approvals: Per comment 18 and comment 26, getting this fix into Macaw would help SugarCRM support Firefox 4.

	John Mertic
	Comment 30 • 13 years ago

Thanks everyone for all your help on this; I really appreciate it!

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 31 • 13 years ago

As far as I can tell, this missed the chance to be in a point release unless the point release is respun with ridealongs.

	Daniel Veditz [:dveditz]
	Comment 33 • 13 years ago

Comment on attachment 522650 [details] [diff] [review]
Flush tree ops in the fragment case when the tree builder yields due to a script, v3

Not doing another 4.0.x release, 5.0 will be out soon.