Last Comment Bug 749367 - <script type="text/template"> content parsed as JavaScript (+E4X)
: <script type="text/template"> content parsed as JavaScript (+E4X)
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 15 Branch
: All All
: -- major (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
Mentors:
: 749627 756715 (view as bug list)
Depends on:
Blocks: 744332 749343
  Show dependency treegraph
 
Reported: 2012-04-26 14:00 PDT by Potch [:potch]
Modified: 2012-08-07 01:59 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch (v1) rough (1.41 KB, patch)
2012-05-03 16:03 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v2) (3.10 KB, patch)
2012-05-03 20:15 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Review
Patch (v3) (6.29 KB, patch)
2012-05-04 04:06 PDT, Mark Capella [:capella]
jst: review+
Ms2ger: feedback+
Details | Diff | Review

Description Potch [:potch] 2012-04-26 14:00:44 PDT
Observed in Nightly 15.0a1 (2012-04-26)

It's become a common pattern [1] in web applications to perform JS templating by placing HTML snippets in a script tag with the type of "text/template". It is said (and assumed by developers) that content wrapped in such a way will not be parsed by browsers or displayed. I observed in the most recent Nightly that HTML inside these script tags is being evaluated as XML and triggering errors in the console. Here is a sample from https://marketplace-dev.allizom.org/en-US/

Timestamp: 4/26/12 1:55:01 PM
Error: SyntaxError: XML can't be the whole program
Source File: https://marketplace-dev.allizom.org/en-US/
Line: 337

This has the potential to cause issues with existing sites.


[1]: https://github.com/documentcloud/backbone/blob/master/examples/todos/index.html
Comment 1 Henri Sivonen (:hsivonen) 2012-05-03 03:39:01 PDT
The problem is that scripts are typed text/template are parsed as JavaScript with E4X enabled.  Since text/template isn't a JavaScript MIME type, the contents of those script elements shouldn't be passed to the JavaScript engine in the first place.
Comment 2 Henri Sivonen (:hsivonen) 2012-05-03 03:48:51 PDT
By code inspection, this is a regression from bug 744332. The else branch at http://hg.mozilla.org/mozilla-central/annotate/807403a04a6a/content/base/src/nsScriptLoader.cpp#l470 is now bogus.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-03 13:34:04 PDT
This is a major web compat bug.  It's common to use <script> tags like this for WebGL too, and it's actually possible that some WebGL shader bits can end up looking sorta like JS, I think.

Mark, do you want to take this?
Comment 4 Mark Capella [:capella] 2012-05-03 15:54:22 PDT
yes... built the patch ... working on a mochitest
Comment 5 Mark Capella [:capella] 2012-05-03 16:03:38 PDT
Created attachment 620879 [details] [diff] [review]
Patch (v1) rough

I may have let this fall out while ms2ger and I were working on separate / related bugs ... here's the rough patch ...

Hoping a mochitest like this should suffice ... I've worked mostly in a11y so where would you say to add this test into? My first thought was to clone perhaps: tests/content/base/test/test_bug166235.html ...

<script type="text/template">
is(true, false, "Shouldn't execute");
</script>

<script type="text/javascript">
is(true, true, "Should execute");
</script>
Comment 6 Mark Capella [:capella] 2012-05-03 20:15:05 PDT
Created attachment 620941 [details] [diff] [review]
Patch (v2)

This patch builds and tests locally. The test fails before, works after.

Running the whole test-suite ends suspiciously fast in my WIN7 environment, though qpop-ing and testing again ends fast also. Maybe something recent due to the PGO link problems ... ?

Have a look, let me know -- mark
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-03 22:38:31 PDT
Comment on attachment 620941 [details] [diff] [review]
Patch (v2)

Two things:

1)  There are other places in the patch for bug 744332 that had similar issues.
2)  I'd structure the test like this:

  <script>
    var ran = false;
  </script>
  <script type="text/template">
    ran = true;
  </script>
  <script>
    is(ran, false, "text/template script should not run");
  </script>

and similar for any other types you want to test.
Comment 8 :Ms2ger 2012-05-04 01:45:12 PDT
Comment on attachment 620941 [details] [diff] [review]
Patch (v2)

Review of attachment 620941 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: content/base/src/nsScriptLoader.cpp
@@ +465,5 @@
>        }
>      }
>  
> +    if (!isJavaScript)
> +      typeID = nsIProgrammingLanguage::UNKNOWN;

{}, please.

::: content/base/test/test_bug749367.html
@@ +16,5 @@
> +  <pre id="test">
> +
> +  <script type="text/template">
> +  is(true, false, "Shouldn't execute");
> +  is(true, true, "Shouldn't execute");

The canonical way to write this is 'ok(false...'. And the second check isn't necessary, right?

@@ +20,5 @@
> +  is(true, true, "Shouldn't execute");
> +  </script>
> +
> +  <script type="text/javascript">
> +  is(true, true, "Should execute");

ok(true
Comment 9 Mark Capella [:capella] 2012-05-04 04:06:41 PDT
Created attachment 621003 [details] [diff] [review]
Patch (v3)

Asking for feedback again ... found another block of identical code in nsXULContentSink.cpp that has now been addressed and a test created for.

Again all builds and individually tests ok locally. The whole test-suite runs ok but seems to finish in about half the time it normally takes, though again, its doing this when I pop my change out also.

Let me know if I should push to try, or go for the review? from JST ...
Comment 10 :Ms2ger 2012-05-04 04:53:33 PDT
Comment on attachment 621003 [details] [diff] [review]
Patch (v3)

Review of attachment 621003 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Comment 11 Mark Capella [:capella] 2012-05-04 14:47:50 PDT
ms2ger: pushed manually
https://tbpl.mozilla.org/?tree=Try&rev=98bd00bb353a
Comment 13 Mounir Lamouri (:mounir) 2012-05-09 09:38:11 PDT
Matt, how is that blocking rinmark? You shouldn't block ringmark but a ring number.
Comment 14 Matt Brubeck (:mbrubeck) 2012-05-09 09:47:38 PDT
This bug does not affect any of the ringmark tests, but it causes errors in the rng.io test-runner HTML page.  I had thought the errors were causing the test runner to fail, but it looks like they are non-fatal, so this probably doesn't need to block.
Comment 15 Ed Morley [:emorley] 2012-05-10 07:38:09 PDT
https://hg.mozilla.org/mozilla-central/rev/4522615e0fc3
Comment 16 Mozilla RelEng Bot 2012-05-11 15:29:57 PDT
Autoland Patchset:
	Patches: 621003
	Branch: mozilla-central => try
Patch 621003 could not be applied to mozilla-central.
patching file content/base/src/nsScriptLoader.cpp
Hunk #1 FAILED at 459
1 out of 1 hunks FAILED -- saving rejects to file content/base/src/nsScriptLoader.cpp.rej
patching file content/base/test/Makefile.in
Hunk #1 FAILED at 572
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/Makefile.in.rej
file content/base/test/test_bug749367.html already exists
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/test_bug749367.html.rej
patching file content/xul/content/test/Makefile.in
Hunk #1 FAILED at 40
1 out of 1 hunks FAILED -- saving rejects to file content/xul/content/test/Makefile.in.rej
file content/xul/content/test/test_bug749367.xul already exists
1 out of 1 hunks FAILED -- saving rejects to file content/xul/content/test/test_bug749367.xul.rej
patching file content/xul/document/src/nsXULContentSink.cpp
Hunk #1 FAILED at 922
1 out of 1 hunks FAILED -- saving rejects to file content/xul/document/src/nsXULContentSink.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-18 19:58:01 PDT
*** Bug 756715 has been marked as a duplicate of this bug. ***
Comment 18 :Ms2ger 2012-05-25 06:33:56 PDT
*** Bug 749627 has been marked as a duplicate of this bug. ***
Comment 19 Ioana (away) 2012-08-07 01:56:41 PDT
Since web apps can only be installed on Nightly builds, I tried to verify this issue on Firefox 15.0 Nightly builds:

05/10 Nightly - the issue still reproduces (the push was only made that day, so this is ok)
05/11 Nightly & 05/27 Nightly - the Install button from the web app pages is unresponsive (nothing happens when clicking it & nothing gets displayed in the Error Console).
Comment 20 Ioana (away) 2012-08-07 01:59:31 PDT
Verified as fixed on the 08/06 Firefox 17.0 Nightly.

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