Last Comment Bug 645115 - With two scripts in innerHTML, the first one runs, even though neither should run
: With two scripts in innerHTML, the first one runs, even though neither should...
Status: RESOLVED FIXED
: html5, testcase
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla5
Assigned To: Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
:
Mentors:
: 646824 655382 663316 666240 (view as bug list)
Depends on:
Blocks: html5-parsing
  Show dependency treegraph
 
Reported: 2011-03-25 11:55 PDT by Clinton Gormley
Modified: 2011-06-22 10:51 PDT (History)
13 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Test case (836 bytes, text/html)
2011-03-25 11:56 PDT, Clinton Gormley
no flags Details
Test case (813 bytes, text/html)
2011-03-25 12:06 PDT, Clinton Gormley
no flags Details
Flush tree ops in the fragment case when the tree builder yields due to a script (3.44 KB, patch)
2011-03-28 00:13 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
no flags Details | Diff | Splinter Review
Flush tree ops in the fragment case when the tree builder yields due to a script, v2 (4.59 KB, patch)
2011-03-28 03:36 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
no flags Details | Diff | Splinter Review
Flush tree ops in the fragment case when the tree builder yields due to a script, v3 (2.96 KB, patch)
2011-03-29 05:42 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
bzbarsky: review+
dveditz: approval2.0-
Details | Diff | Splinter Review

Description Clinton Gormley 2011-03-25 11:55:06 PDT
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
Comment 1 Clinton Gormley 2011-03-25 11:56:11 PDT
Created attachment 521891 [details]
Test case
Comment 2 Clinton Gormley 2011-03-25 12:06:46 PDT
Created attachment 521897 [details]
Test case

Slightly simpler test case
Comment 3 j.j. 2011-03-25 14:06:25 PDT
Should not execute at all!?
Other browsers don't, Fx3.6 did in both cases.
Comment 4 Ed Morley [:emorley] 2011-03-25 14:27:44 PDT
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?
Comment 5 Clinton Gormley 2011-03-25 14:38:23 PDT
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.
Comment 6 Boris Zbarsky [:bz] 2011-03-25 14:48:33 PDT
Uh... this looks broken.  Henri, what's going on?
Comment 7 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-28 00:13:45 PDT
Created attachment 522311 [details] [diff] [review]
Flush tree ops in the fragment case when the tree builder yields due to a script

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.
Comment 8 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-28 00:16:33 PDT
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.)
Comment 9 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-28 03:36:50 PDT
Created attachment 522327 [details] [diff] [review]
Flush tree ops in the fragment case when the tree builder yields due to a script, v2

Now with the mochitest actually hg added.
Comment 10 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-29 00:08:38 PDT
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.
Comment 11 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-29 05:42:30 PDT
Created attachment 522650 [details] [diff] [review]
Flush tree ops in the fragment case when the tree builder yields due to a script, v3

The fix itself was ok. The assertions I added were bogus, so I removed the assertions.
Comment 12 Boris Zbarsky [:bz] 2011-03-31 07:04:04 PDT
*** Bug 646824 has been marked as a duplicate of this bug. ***
Comment 13 Boris Zbarsky [:bz] 2011-03-31 07:19:50 PDT
*** Bug 646824 has been marked as a duplicate of this bug. ***
Comment 14 Boris Zbarsky [:bz] 2011-04-05 18:17:30 PDT
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.
Comment 15 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-04-05 23:01:30 PDT
(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
Comment 17 Daniel Veditz [:dveditz] 2011-04-07 17:59:48 PDT
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.
Comment 18 John Mertic 2011-04-12 12:46:51 PDT
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.
Comment 19 Boris Zbarsky [:bz] 2011-04-12 13:41:25 PDT
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?
Comment 20 John Mertic 2011-04-12 19:20:52 PDT
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.
Comment 21 Boris Zbarsky [:bz] 2011-04-12 20:31:28 PDT
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.
Comment 22 Boris Zbarsky [:bz] 2011-04-12 20:35:01 PDT
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.
Comment 23 John Mertic 2011-04-12 21:06:33 PDT
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.
Comment 24 Boris Zbarsky [:bz] 2011-04-12 21:49:39 PDT
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.
Comment 26 John Mertic 2011-04-13 19:48:11 PDT
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?
Comment 27 Boris Zbarsky [:bz] 2011-04-13 19:54:31 PDT
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?
Comment 28 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-04-13 23:55:07 PDT
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.
Comment 29 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-04-13 23:59:28 PDT
Note to the person processing approvals: Per comment 18 and comment 26, getting this fix into Macaw would help SugarCRM support Firefox 4.
Comment 30 John Mertic 2011-04-14 18:27:43 PDT
Thanks everyone for all your help on this; I really appreciate it!
Comment 31 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-04-15 05:03:33 PDT
As far as I can tell, this missed the chance to be in a point release unless the point release is respun with ridealongs.
Comment 32 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-05-08 08:09:01 PDT
*** Bug 655382 has been marked as a duplicate of this bug. ***
Comment 33 Daniel Veditz [:dveditz] 2011-06-03 16:45:48 PDT
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.
Comment 34 Boris Zbarsky [:bz] 2011-06-09 22:29:22 PDT
*** Bug 663316 has been marked as a duplicate of this bug. ***
Comment 35 Boris Zbarsky [:bz] 2011-06-22 10:51:34 PDT
*** Bug 666240 has been marked as a duplicate of this bug. ***

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