The default bug view has changed. See this FAQ.

With two scripts in innerHTML, the first one runs, even though neither should run

RESOLVED FIXED in mozilla5

Status

()

Core
HTML: Parser
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Clinton Gormley, Assigned: hsivonen)

Tracking

({html5, testcase})

Trunk
mozilla5
html5, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 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
(Reporter)

Updated

6 years ago
Version: unspecified → 4.0 Branch
(Reporter)

Comment 1

6 years ago
Created attachment 521891 [details]
Test case
(Reporter)

Comment 2

6 years ago
Created attachment 521897 [details]
Test case

Slightly simpler test case
Attachment #521891 - Attachment is obsolete: true

Comment 3

6 years ago
Should not execute at all!?
Other browsers don't, Fx3.6 did in both cases.
Severity: major → normal
Component: General → HTML: Parser
Keywords: html5
OS: Linux → All
Product: Firefox → Core
QA Contact: general → parser
Hardware: x86_64 → All
Version: 4.0 Branch → Trunk
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?
Blocks: 373864
(Reporter)

Comment 5

6 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.
Uh... this looks broken.  Henri, what's going on?
Assignee: nobody → hsivonen
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
(Assignee)

Comment 7

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

Comment 8

6 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.)
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Summary: script tag added with innerHTML requires a second script tag before it is executed → With two scripts in innerHTML, the first one runs, even though neither should run
(Assignee)

Comment 9

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

Comment 10

6 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.
Attachment #522327 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

6 years ago
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.
Attachment #522327 - Attachment is obsolete: true
Attachment #522650 - Flags: review?(bzbarsky)
Duplicate of this bug: 646824
Duplicate of this bug: 646824

Updated

6 years ago
Attachment #522650 - Attachment is patch: true
Attachment #522650 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #522650 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

6 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
Whiteboard: [fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/0e91598cc3f9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
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.
blocking2.0: ? → -

Comment 18

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

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

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

Comment 26

6 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?
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?
(Assignee)

Comment 28

6 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.
Attachment #522650 - Flags: approval2.0?
(Assignee)

Comment 29

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

Comment 30

6 years ago
Thanks everyone for all your help on this; I really appreciate it!
(Assignee)

Comment 31

6 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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 655382
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.
Attachment #522650 - Flags: approval2.0? → approval2.0-
Duplicate of this bug: 663316
Duplicate of this bug: 666240
You need to log in before you can comment on or make changes to this bug.